Skip to content

Restore labels when re-creating Windows networks#49196

Merged
robmry merged 1 commit intomoby:masterfrom
robmry:49179_restore_windows_network_labels
Jan 2, 2025
Merged

Restore labels when re-creating Windows networks#49196
robmry merged 1 commit intomoby:masterfrom
robmry:49179_restore_windows_network_labels

Conversation

@robmry
Copy link
Copy Markdown
Contributor

@robmry robmry commented Jan 2, 2025

- What I did

- How I did it

Save and restore network labels when re-creating Windows networks during daemon startup.

- How to verify it

As described in the issue ...

docker network create --label test --driver nat test
docker network inspect test. Check that the label test has been successfully added.
Restart the Windows service for the docker engine ( or restart the computer to install OS updates)
docker network inspect test. Check if the label test still exists.

- Description for the changelog

- Preserve network labels during daemon startup.

@robmry robmry added this to the 28.0.0 milestone Jan 2, 2025
@robmry robmry self-assigned this Jan 2, 2025
@robmry robmry marked this pull request as ready for review January 2, 2025 12:15
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

Some questions;

  • Do we know if this impacts older versions as well? (if so, we should add cherry-pick labels for those probably)
  • I'm guessing "it's complicated", but do we have any tests on Windows that restart the daemon? (I know testing on windows is more complicated, because we can't spin up daemons "per test", so not sure if we can easily test, but if there's an existing test that does a restart, perhaps we should have one for this as well).
  • Are there other custom options besides "Labels" and "Options" for Windows networks? (mostly considering; would we have some way to catch regressions if there would ever be other options?)

@robmry
Copy link
Copy Markdown
Contributor Author

robmry commented Jan 2, 2025

  • Do we know if this impacts older versions as well? (if so, we should add cherry-pick labels for those probably)

Yes, all older versions I think ... I added a cherry-pick label for 27.x, I'll add 25.0 and 26.1 too - although I'm not sure we'll want to update those.

  • I'm guessing "it's complicated", but do we have any tests on Windows that restart the daemon? (I know testing on windows is more complicated, because we can't spin up daemons "per test", so not sure if we can easily test, but if there's an existing test that does a restart, perhaps we should have one for this as well).

Yes, it's a wider problem ... the test suites start their own daemon, and it's not currently possible to start another on the same host as we do for Linux. Perhaps we should have a Windows test suite that doesn't start its own daemon, so that tests can manage their own - but that's out of scope here.

  • Are there other custom options besides "Labels" and "Options" for Windows networks? (mostly considering; would we have some way to catch regressions if there would ever be other options?)

Nothing jumped out at me but yes, there may be other things that get lost when these networks are re-created. If we could write an integration test for it, comparing before/after inspect outputs would be good - although it'd only catch missing attributes populated by the test. (I'm not sure how we'd write a test that could catch dropped options that don't exist yet.)

@robmry
Copy link
Copy Markdown
Contributor Author

robmry commented Jan 2, 2025

I'll go ahead and merge this ... it fixes the reported problem, and we should look at improvements to regression testing infrastructure separately.

@thaJeztah
Copy link
Copy Markdown
Member

Thanks! Yes, definitely fine to go otherwise. I also can't come up with an easy way to address the Windows testing situation; not having DIND available as option, and starting multiple daemons being less than trivial for sure makes some things more complicated.

@robmry robmry deleted the 49179_restore_windows_network_labels branch January 3, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Network loses labels after docker service restart on Windows

3 participants