-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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(minikube) display scheduledstop status in minikube status #9793
feat(minikube) display scheduledstop status in minikube status #9793
Conversation
Hi @tharun208. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Can one of the admins verify this patch? |
bb0a58f
to
07083d4
Compare
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.
do u mind adding before/after this PR status output ?
in both cases when it is scheduled and not scheduled
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.
Thank you for taking this issue on! Left a few comments, please let me know if you have any questions.
0a54a21
to
5f5fcf2
Compare
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.
Thanks for the update, left a suggestion,
5f5fcf2
to
9381646
Compare
@priyawadhwa It is working as expected. I will now add a check-in |
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.
An integration test would be great!
Could you also provide the output of:
minikube start
minikube stop --schedule 2m
minikube status
minikube status --output json --layout cluster
Instead of writing a new test, you can actually just add a check for
which basically just makes sure that minikube/test/integration/scheduled_stop_test.go Lines 86 to 90 in 8fc54b4
|
/ok-to-test |
kvm2 Driver |
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.
Thank you for your changes! I tested locally and it's really close, just a couple more suggestions and we should be ready to merge :)
Signed-off-by: Tharun <rajendrantharun@live.com>
Signed-off-by: Tharun <rajendrantharun@live.com>
Signed-off-by: Tharun <rajendrantharun@live.com>
kvm2 Driver Times for Minikube (PR 9793): 62.9s 65.8s 65.5s Averages Time Per Log
docker Driver Times for Minikube (PR 9793): 29.3s 30.4s 31.7s Averages Time Per Log
|
97af926
to
d6a49a5
Compare
Signed-off-by: Tharun <rajendrantharun@live.com>
d6a49a5
to
c51a204
Compare
kvm2 Driver Times for Minikube (PR 9793): 63.4s 65.6s 64.4s Averages Time Per Log
docker Driver Times for Minikube (PR 9793): 29.3s 27.6s 30.4s Averages Time Per Log
|
Signed-off-by: Tharun <rajendrantharun@live.com>
kvm2 Driver Times for Minikube (PR 9793): 64.7s 65.9s 63.1s Averages Time Per Log
docker Driver Times for Minikube (PR 9793): 30.4s 32.1s 29.8s Averages Time Per Log
|
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.
Thank you! This is looking really good, just one more comment.
@@ -84,6 +87,8 @@ func TestScheduledStopUnix(t *testing.T) { | |||
|
|||
// schedule a stop for 5 min from now and make sure PID is created | |||
stopMinikube(ctx, t, profile, []string{"--schedule", "5m"}) | |||
// make sure timeToStop is present in status | |||
ensureMinikubeStatus(ctx, t, profile, "TimeToStop", "3m") |
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.
I see that you had to change to using strings.Contains
in the ensureMinikubeStatus
function, since we can't know what the exact value of TimeToStop
will be. Instead of doing a contains, which could potentially fail, I would just check that TimeToStop is a valid duration < 5m.
Then we can also revert back to making sure got == wantStatus
in the ensureMinikubeStatus
function
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.
So, can we pull this check to a seperate function or modify the comparison based on the key
value to the ensureMinikubeStatus
itself ?.
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.
Let's do a separate function
Signed-off-by: Tharun <rajendrantharun@live.com>
kvm2 Driver Times for Minikube (PR 9793): 62.3s 62.9s 63.4s Averages Time Per Log
docker Driver Times for Minikube (PR 9793): 28.5s 30.2s 29.0s Averages Time Per Log
|
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.
Looks good, thank you so much for contributing this feature!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: priyawadhwa, tharun208 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Tharun rajendrantharun@live.com
Updated minikube status command to show
scheduledStop
timeFixes #9731