-
Notifications
You must be signed in to change notification settings - Fork 110
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
RSDK-8701 Show configuring state through MachineStatus endpoint #4359
RSDK-8701 Show configuring state through MachineStatus endpoint #4359
Conversation
t.Run("poll during reconfiguration", func(t *testing.T) { | ||
rev1 := "rev1" | ||
lr := setupLocalRobot(t, ctx, &config.Config{ | ||
Revision: rev1, | ||
Components: []resource.Config{newMockConfig("m", 200, false, "")}, | ||
}, logger) | ||
|
||
// update resource with a working config that is slow to reconfigure. | ||
rev2 := "rev2" | ||
var wg sync.WaitGroup | ||
wg.Add(1) | ||
go func() { | ||
defer wg.Done() | ||
lr.Reconfigure(ctx, &config.Config{ | ||
Revision: rev2, | ||
Components: []resource.Config{newMockConfig("m", 300, false, "1s")}, | ||
}) | ||
}() | ||
// sleep for a short amount of time to allow the machine to receive a new | ||
// revision. this sleep should be shorter than the resource update duration | ||
// defined above so that updated resource is still in a "configuring" state. | ||
time.Sleep(time.Millisecond * 100) | ||
|
||
// get status while reconfiguring | ||
mStatus, err := lr.MachineStatus(ctx) | ||
test.That(t, err, test.ShouldBeNil) | ||
test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev2) | ||
|
||
// the component whose config changed should be the only component in a | ||
// "configuring" state and associated with the original revision. | ||
filterConfiguring := rtestutils.FilterByStatus(t, mStatus.Resources, resource.NodeStateConfiguring) | ||
expectedConfiguring := []resource.Status{ | ||
{ | ||
Name: mockNamed("m"), | ||
State: resource.NodeStateConfiguring, | ||
Revision: rev1, | ||
}, | ||
} | ||
rtestutils.VerifySameResourceStatuses(t, filterConfiguring, expectedConfiguring) | ||
|
||
// all other components should be in the "ready" state and associated with the | ||
// new revision. | ||
filterReady := rtestutils.FilterByStatus(t, mStatus.Resources, resource.NodeStateReady) | ||
expectedReady := getExpectedDefaultStatuses(rev2) | ||
rtestutils.VerifySameResourceStatuses(t, filterReady, expectedReady) | ||
|
||
wg.Wait() | ||
|
||
// get status after reconfigure finishes | ||
mStatus, err = lr.MachineStatus(ctx) | ||
test.That(t, err, test.ShouldBeNil) | ||
test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev2) | ||
|
||
// now all components, including the one whose config changed, should all be in | ||
// the "ready" state and associated with the new revision. | ||
expectedStatuses := rtestutils.ConcatResourceStatuses( | ||
getExpectedDefaultStatuses(rev2), | ||
[]resource.Status{ | ||
{ | ||
Name: mockNamed("m"), | ||
State: resource.NodeStateReady, | ||
Revision: rev2, | ||
}, | ||
}, | ||
) | ||
rtestutils.VerifySameResourceStatuses(t, mStatus.Resources, expectedStatuses) | ||
}) |
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.
pardon the noisy diff - the test highlighted with this comment is the only new test. the remaining changes are refactors.
this test correctly fails if the locked removed in commit 3556889 is reinstated.
a8f90e5
to
1f34a19
Compare
// sleep for a short amount of time to allow the machine to receive a new | ||
// revision. this sleep should be shorter than the resource update duration | ||
// defined above so that updated resource is still in a "configuring" state. | ||
time.Sleep(time.Millisecond * 100) |
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.
relying on a sleep is a little janky - alternatively we could poll machine status until the revision on the machine is updated and fail after some attempts. any other suggestions are welcome here.
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.
can do a WaitForAssertion
// sleep for a short amount of time to allow the machine to receive a new | ||
// revision. this sleep should be shorter than the resource update duration | ||
// defined above so that updated resource is still in a "configuring" state. | ||
time.Sleep(time.Millisecond * 100) |
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.
can do a WaitForAssertion
3556889
to
f4628a8
Compare
This reverts commit f4628a8.
Stop holding
resourceGraphLock
while fetching machine status from local robot, since this prevents users from seeing resources statuses while a robot is (re)configuring.