Skip to content

sources/azure: remove lease file parsing#1302

Merged
TheRealFalcon merged 1 commit into
canonical:mainfrom
cjp256:azure-remove-lease-file-parsing
Mar 8, 2022
Merged

sources/azure: remove lease file parsing#1302
TheRealFalcon merged 1 commit into
canonical:mainfrom
cjp256:azure-remove-lease-file-parsing

Conversation

@cjp256
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 commented Feb 24, 2022

With reporting ready now happening in local phase, we have access
to ephemeral DHCP lease options and no longer need to parse DHCP
lease files.

  • Switch from tracking wireserver endpoint in its encoded form to the
    IP string, parsing it only when read from lease options.

  • Drop fallback_lease_file and dhcp_options parameters in favor of
    processed endpoint string.

  • Add some minor type information for mypy.

Signed-off-by: Chris Patterson cpatterson@microsoft.com

Copy link
Copy Markdown
Contributor

@anhvoms anhvoms left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM! I'm going to close/re-open to re-trigger CI

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Looks like there's still some broken tests.

@cjp256 cjp256 force-pushed the azure-remove-lease-file-parsing branch from 6ab8f29 to 0af26e1 Compare March 8, 2022 12:18
@cjp256
Copy link
Copy Markdown
Contributor Author

cjp256 commented Mar 8, 2022

Fixed tests & rebased

@cjp256 cjp256 force-pushed the azure-remove-lease-file-parsing branch from 0af26e1 to 079222f Compare March 8, 2022 13:12
@cjp256
Copy link
Copy Markdown
Contributor Author

cjp256 commented Mar 8, 2022

There seems to be an unrelated failure in the tests:

________________________ TestCombined.test_no_problems _________________________
self = <test_combined.TestCombined object at 0x7f93674a3278>
class_client = <tests.integration_tests.instances.IntegrationInstance object at 0x7f9367550048>
    def test_no_problems(self, class_client: IntegrationInstance):
        """Test no errors, warnings, or tracebacks"""
        client = class_client
        status_file = client.read_from_file("/run/cloud-init/status.json")
        status_json = json.loads(status_file)["v1"]
        for stage in ("init", "init-local", "modules-config", "modules-final"):
>           assert status_json[stage]["errors"] == []
E           assert ['(\'ssh-impo...Stderr: -"))'] == []
E             Left contains one more item: '(\'ssh-import-id\', ProcessExecutionError("Unexpected error while running command.\\nCommand: [\'sudo\', \'-Hu\', \'ubuntu\', \'ssh-import-id\', \'gh:powersj\', \'lp:smoser\']\\nExit code: 1\\nReason: -\\nStdout: -\\nStderr: -"))'
E             Full diff:
E               [
E             -  ,
E             +  '(\'ssh-import-id\', ProcessExecutionError("Unexpected error while running '
E             +  "command.\\nCommand: ['sudo', '-Hu', 'ubuntu', 'ssh-import-id', 'gh:powersj', "
E             +  '\'lp:smoser\']\\nExit code: 1\\nReason: -\\nStdout: -\\nStderr: -"))',...
E             
E             ...Full output truncated (2 lines hidden), use '-vv' to show
tests/integration_tests/modules/test_combined.py:194: AssertionError
---------------------------- Captured stdout setup -----------------------------
Detected image: image_id=focal os=ubuntu release=focal

@holmanb
Copy link
Copy Markdown
Member

holmanb commented Mar 8, 2022

There seems to be an unrelated failure in the tests:

This is the second time I've seen that failure in the last this week. I think this is a flaky test that should be removed from CI. This depends on multiple keyservers so remote failure is likely cause, not your changes. A no-op push should get tests passing. I am proposing a separate PR to remove that test from CI runs.

@TheRealFalcon
Copy link
Copy Markdown
Contributor

A no-op push should get tests passing.

Already restarted them

@cjp256
Copy link
Copy Markdown
Contributor Author

cjp256 commented Mar 8, 2022

It failed twice this morning so I had just pushed -vv to the tests for better visibility.

@TheRealFalcon
Copy link
Copy Markdown
Contributor

We're putting up a PR to (re)move the test. It's failing because ssh-import-id is failing, which would be due to the Launchpad or Github service being down and unrelated to cloud-init.

@cjp256 cjp256 force-pushed the azure-remove-lease-file-parsing branch from 3934889 to 079222f Compare March 8, 2022 14:42
@TheRealFalcon
Copy link
Copy Markdown
Contributor

I see that it failed again which indicates a different problem, but it is still a problem with ssh-import-id and not cloud-init, so I still moved the test out of CI. If you rebase, we should be able to pass now.

@cjp256 cjp256 force-pushed the azure-remove-lease-file-parsing branch from 079222f to bf49885 Compare March 8, 2022 15:01
With reporting ready now happening in local phase, we have access
to ephemeral DHCP lease options and no longer need to parse DHCP
lease files.

- Switch from tracking wireserver endpoint in its encoded form to the
  IP string, parsing it only when read from lease options.

- Drop fallback_lease_file and dhcp_options parameters in favor of
  processed endpoint string.

- Add some minor type information for mypy.

- Update various tests.

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
@cjp256
Copy link
Copy Markdown
Contributor Author

cjp256 commented Mar 8, 2022

Done

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks!

@TheRealFalcon TheRealFalcon merged commit 5ad0768 into canonical:main Mar 8, 2022
@cjp256 cjp256 deleted the azure-remove-lease-file-parsing branch April 25, 2022 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants