Skip to content

cli: cloud-id report not-run or disabled state as cloud-id#1162

Merged
TheRealFalcon merged 6 commits into
canonical:mainfrom
blackboxsw:cli-handle-not-run-ephemeral
Jan 13, 2022
Merged

cli: cloud-id report not-run or disabled state as cloud-id#1162
TheRealFalcon merged 6 commits into
canonical:mainfrom
blackboxsw:cli-handle-not-run-ephemeral

Conversation

@blackboxsw
Copy link
Copy Markdown
Collaborator

  • cli: status avoid reporting disabled before generator runs
    

    Once cloud-init's systemd generator runs it emits either:
    /run/cloud-init/enabled OR /run/cloud-init/disabled

    If we have neither of those files, cloud-init status should
    still report not-run.

  • cli: cloud-id report not-run or disabled state as cloud-id
    

    Given that cloud-id could be run before cloud-init discovery has
    run report when cloud-init is in a 'disabled' state or whether it
    has 'not-run'.

    Adopt new exit codes:
    0: success
    1: error
    2: cloud-init is in disabled state
    3: cloud-init generator has not run yet

Proposed Commit Message

    cli: cloud-id report not-run or disabled state as cloud-id

    This fix has two elements:
     - cloud-init status will not correctly report 'not-run' prior to systemd
       generator running. Only report "disabled" when generator has run
       and /run/cloud-init/disabled exists.
     -  Expose not-run and disabled state in cloud-id responses
     -  Add unique error codes from cloud-id for error, disabled and not-run.
    
   The new cloud-id exit codes:
     0: success
     1: error
     2: cloud-init is in disabled state
     3: cloud-init generator has not run yet

Additional Context

Test Steps

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

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.

One inline nit. I think we should add a couple of tests to test_cloud_id.py for the new return codes.

Additionally, should we be concerned about backwards compatibility?

Comment thread cloudinit/cmd/cloud_id.py Outdated
Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

I added a couple of minor comments (mostly nits). This looks good to me so far.

Comment thread tests/unittests/cmd/test_status.py
Comment thread cloudinit/cmd/status.py Outdated
class UXAppStatus(enum.Enum):
"""Enum representing user-visible cloud-init application status."""

ENABLED_NOT_RUN = "not-run"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Idea: Besides "not-run" the enum names are just the lower-case equivalent of the enum name. If they all matched, I would suggest making the return codes the enum value to store the rc alongside the return message (and then access the enum via enum.name or enum.name.lower() when needed for printing messages).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm probably not understanding the intent of your comment here. If get_status_details returned the lower enum value to the caller handle_status_args wouldn't I then have to decode those via UXAppStatus.name.lower() here too

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You would, yes. The point I'm making is that we define an enum data structure that contains strings and later use the value of the enum to derive error codes here.

This suggestion was intended to be "rather than derive error codes programmatically why not to store them together with their intended string representation, since it appears that we have a 1:1 mapping". The specifics I mentioned with .lower() is one way to do that given the current enum usage. Also this suggestion might be unnecessary for a data structure that isn't widely used - I'm fine with it as is if you prefer it the way it was.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a really good pt @holman. I thought I would adopt this approach as suggested, but forgot that RUNNING vs DONE status types would have the common non-unique exit code of 0. There may be other enum status that grow over time that also might fall into exit-code 1 for general failure. So, I think I won't be able to use enum to represent this structure.

Comment thread cloudinit/cmd/status.py
@blackboxsw blackboxsw force-pushed the cli-handle-not-run-ephemeral branch from 53cd7c7 to 012a80a Compare January 5, 2022 23:32
Once cloud-init's systemd generator runs it emits either:
   /run/cloud-init/enabled  OR /run/cloud-init/disabled

If we have neither of those files, cloud-init status should
still report not-run.
Given that cloud-id could be run before cloud-init discovery has
run report when cloud-init is in a 'disabled' state or whether it
has 'not-run'.

Adopt new exit codes:
 0: success
 1: error
 2: cloud-init is in disabled state
 3: cloud-init generator has not run yet
@blackboxsw blackboxsw force-pushed the cli-handle-not-run-ephemeral branch from 012a80a to e2dfb39 Compare January 11, 2022 15:06
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, thanks!

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.

3 participants