-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow disabling of network activation (SC-307) #1048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9532c06
Allow disabling of network activation
TheRealFalcon ca166cd
make it a function
TheRealFalcon 7575f93
unit test
TheRealFalcon fe31281
review comments
TheRealFalcon 0e02eaf
review
TheRealFalcon 359fb4b
Merge branch 'main' into unbringup-interfaces
blackboxsw 3bb9f35
Merge branch 'main' into unbringup-interfaces
blackboxsw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,8 +227,11 @@ def apply_network_config(self, netconfig, bring_up=False) -> bool: | |
|
|
||
| # Now try to bring them up | ||
| if bring_up: | ||
| LOG.debug('Bringing up newly configured network interfaces') | ||
| network_activator = activators.select_activator() | ||
| network_activator.bring_up_all_interfaces(network_state) | ||
| else: | ||
| LOG.debug("Not bringing up newly configured network interfaces") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good log tracers here for quick triage in failure cases if needed. |
||
| return False | ||
|
|
||
| def apply_network_config_names(self, netconfig): | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
43 changes: 43 additions & 0 deletions
43
tests/integration_tests/datasources/test_network_dependency.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import pytest | ||
|
|
||
| from tests.integration_tests.clouds import IntegrationCloud | ||
| from tests.integration_tests.conftest import get_validated_source | ||
|
|
||
|
|
||
| def _setup_custom_image(session_cloud: IntegrationCloud): | ||
| """Like `setup_image` in conftest.py, but with customized content.""" | ||
| source = get_validated_source(session_cloud) | ||
| if not source.installs_new_version(): | ||
| return | ||
| client = session_cloud.launch() | ||
|
|
||
| # Insert our "disable_network_activation" file here | ||
| client.write_to_file( | ||
| '/etc/cloud/cloud.cfg.d/99-disable-network-activation.cfg', | ||
| 'disable_network_activation: true\n', | ||
| ) | ||
|
|
||
| client.install_new_cloud_init(source) | ||
| # Even if we're keeping instances, we don't want to keep this | ||
| # one around as it was just for image creation | ||
| client.destroy() | ||
|
|
||
|
|
||
| # This test should be able to work on any cloud whose datasource specifies | ||
| # a NETWORK dependency | ||
| @pytest.mark.gce | ||
| @pytest.mark.ubuntu # Because netplan | ||
| def test_network_activation_disabled(session_cloud: IntegrationCloud): | ||
| """Test that the network is not activated during init mode.""" | ||
| _setup_custom_image(session_cloud) | ||
| with session_cloud.launch() as client: | ||
| result = client.execute('systemctl status google-guest-agent.service') | ||
| if not result.ok: | ||
| raise AssertionError('google-guest-agent is not active:\n%s', | ||
| result.stdout) | ||
| log = client.read_from_file('/var/log/cloud-init.log') | ||
|
|
||
| assert "Running command ['netplan', 'apply']" not in log | ||
|
|
||
| assert 'Not bringing up newly configured network interfaces' in log | ||
| assert 'Bringing up newly configured network interfaces' not in log |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test coverage here. It's functional for the additional function created. Since main isn't covered well with unit tests, this makes things tough to instrument unit tests for this interaction so I think its fine we rely on the integration test here.