Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Zdevs autoinstall #2167

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Zdevs autoinstall #2167

wants to merge 3 commits into from

Conversation

ogayot
Copy link
Member

@ogayot ogayot commented Feb 24, 2025

This is to support doing basic actions (i.e., enable/disable) on zdevs with autoinstall. The work is a joint effort with @Chris-Peterson444.

zdevs:
  - id: 0.0.1506
    state: enabled
  - id: 0.0.1507
    state: enabled
  - id: 0.0.1508
    state: disabled
  # or using an alternative YAML syntax
  - {id: 0.0.1509, state: enabled}

In the first implementation we did, we used separate sections for disable and enable, but the order is important for DASDs.

Chris-Peterson444
Chris-Peterson444 approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@Chris-Peterson444 Chris-Peterson444 left a comment

Choose a reason for hiding this comment

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

This LGTM. Thanks for the extra tests too.

I typically like the multi-commit merge but I think this one could be squashed into a single commit. What do you think?

dbungert
dbungert reviewed Feb 24, 2025
View reviewed changes
Copy link
Collaborator

@dbungert dbungert left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I have a few suggestions to discuss.

subiquity/server/controllers/zdev.py Outdated
@@ -593,10 +596,43 @@
id="0.0.c0fe" type="generic-ccw" on="no" exists="yes" pers="no" auto="no" failed="yes" names=""''' # noqa: E501


ZdevAiItem = TypedDict(
Copy link
Collaborator

Choose a reason for hiding this comment

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

For most things like this we been doing attrs, what is the motivation here for TypedDict instead?

Copy link
Member Author

@ogayot ogayot Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The main point of the typedict is to use it as a type hint for make_autoinstall and load_autoinstall_data. But I can convert this data to an attr to store in self.ai_data and self.actions_done

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it. I still lean towards the attrs implementation to stay consistent with other places in Subiquity. Can we try it in a new commit and see if it reads differently?

Copy link
Member Author

Choose a reason for hiding this comment

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

Chris and I updated the code to use attrs as well (in the last commit)

subiquity/server/controllers/zdev.py
on = False
state = "disabled"
else:
raise ValueError("action must be 'enable' or 'disable'")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
raise ValueError("action must be 'enable' or 'disable'")
raise AutoinstallError("action must be 'enable' or 'disable'")

Copy link
Member Author

Choose a reason for hiding this comment

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

chzdev is also called by non-autoinstall code. Maybe we catch the ValueError and wrap it in an AutoinstallError in autoinstall code.

subiquity/server/controllers/zdev.py Outdated
"ZdevAiItem",
{
"id": str,
"state": Literal["enabled", "disabled"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggest using enable/disable here to match the apidef and to indicate that we are taking this action, not asserting initial state matches expectations.

Copy link
Member Author

Choose a reason for hiding this comment

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

we had the following in our previous draft:

"id": str
"action": Literal["enable", "disable"]

Let's discuss if we think it's better

subiquity/server/controllers/zdev.py Outdated
else:
chzdev_cmd = ["chzdev", "--%s" % action, zdev.id]
await arun_command(chzdev_cmd)
await asyncio.sleep(random.random() * 0.4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
await asyncio.sleep(random.random() * 0.4)
await asyncio.sleep(random.random() * 0.4 / self.app.scale_factor)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I guess that would make sense. I can even configure the delay directly in DryRunCommandRunner._get_delay_for_cmd and skip the call to asyncio.sleep here.

@ogayot
Copy link
Member Author

ogayot commented Feb 25, 2025

I typically like the multi-commit merge but I think this one could be squashed into a single commit. What do you think?

Yeah, I'll rework the history a bit. I want to make the move arun_command to CommandRunner a separate change, for instance.

@ogayot
zdev: use command runner to benefit from dry-run implementation
174fbd5 
Previously, in the Zdev controller, we had code to make sure that we didn't
call `chzdev` when running in dry-run mode. Instead we would call
asyncio.sleep.

We can make the code simpler though by using CommandRunner. It has a dry-run
implementation that does not actually execute commands - and allows configuring
a sleep duration.

Signed-off-by: Olivier Gayot <[email protected]>
@ogayot ogayot force-pushed the zdev-autoinstall branch 2 times, most recently from 2295d7f to 534c7e6 Compare February 25, 2025 14:11
dbungert
dbungert reviewed Feb 25, 2025
View reviewed changes
doc/reference/autoinstall-reference.rst
Controls whether the device should be enabled or disabled. Supported values are:

* ``enabled``
* ``disabled``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's discuss if we want to support ignore, and if not, should we be treating this more as a yaml boolean?

@ogayot
zdev: add autoinstall for enabling and disabling devices
6892bc7 
This introduces a new "zdevs" autoinstall, which makes it possible to enable or
disable supported devices on IBM Z.

One can supply a "zdevs" autoinstall section in the following format:

zdevs:
  - id: 0.0.1506
    state: enabled
  - id: 0.0.1507
    state: enabled
  - id: 0.0.1508
    state: disabled

This is a list since the ordering of operations is important.

Signed-off-by: Olivier Gayot <[email protected]>
@ogayot ogayot force-pushed the zdev-autoinstall branch from afba6ef to e26aa29 Compare March 3, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbungert dbungert dbungert left review comments

@Chris-Peterson444 Chris-Peterson444 Chris-Peterson444 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ogayot @dbungert @Chris-Peterson444