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.

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

feat: implement power states as machine stage events #843

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

utkuozdemir
Copy link
Member

Introduce a new component to watch infra.MachineStatus resources to produce complementary "synthetic" machine events for "powering on" and "powered off" stages.

Change the logic in MachineStatusSnapshot and ClusterMachineStatus controllers to take these changes into account.

This is required to display the correct status for the machines that are powered on/off by the infra providers.

Introduce a new component to watch infra.MachineStatus resources to produce complementary "synthetic" machine events for "powering on" and "powered off" stages.

Change the logic in `MachineStatusSnapshot` and `ClusterMachineStatus` controllers to take these changes into account.

This is required to display the correct status for the machines that are powered on/off by the infra providers.

Signed-off-by: Utku Ozdemir <[email protected]>
if err != nil {
return fmt.Errorf("failed to read resource from the event: %w", err)
}

Copy link
Member

@Unix4ever Unix4ever Jan 16, 2025

Choose a reason for hiding this comment

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

I think we might need to ignore Destroyed events here.

Or make it the opposite: only Created and Updated should be handled.

@@ -32,17 +32,19 @@ type MachineStatusSnapshotController struct {
runner *task.Runner[snapshot.InfoChan, snapshot.CollectTaskSpec]
notifyCh chan *omni.MachineStatusSnapshot
siderolinkCh <-chan *omni.MachineStatusSnapshot
powerStageCh <-chan *omni.MachineStatusSnapshot
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could reuse siderolinkCh here instead of creating a new channel.

Copy link
Member Author

@utkuozdemir utkuozdemir Jan 16, 2025

Choose a reason for hiding this comment

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

I just thought this would be clearer, as they don't come from SideroLink. I think it is more obvious this way that there are 2 different sources. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

or we could rename siderolinkCh to the eventsCh, as they are kinda similar.
I'm torn now :D

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can keep the two channels

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.

3 participants