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
Bug 1976894: Idling a StatefulSet seems to work however accessing the Services Route does not wake up the application ("Application is not available" error page is returned). #1026
Conversation
@miheer: This pull request references Bugzilla bug 1976894, which is invalid:
Comment In response to this:
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. |
/bugzilla refresh |
@miheer: This pull request references Bugzilla bug 1976894, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
8a2642b
to
251b6db
Compare
pkg/cli/idle/idle.go
Outdated
fmt.Println("Idling StatefulSet is not supported yet.") | ||
return nil, nil |
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.
Is there a reason you need to print here instead of returning an error?
fmt.Println("Idling StatefulSet is not supported yet.") | |
return nil, nil | |
return nil, fmt.Errorf("idling StatefulSets is not supported") |
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.
Also, don't you need three return values here?
fmt.Println("Idling StatefulSet is not supported yet.") | |
return nil, nil | |
return nil, nil, fmt.Errorf("idling StatefulSets is not supported") |
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 @Miciah ! return is inside getController := func(ref namespacedOwnerReference) (metav1.Object, error) {
so I will add return nil, fmt.Errorf("idling StatefulSets is not supported") as can't return 3 values.
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.
Is there a reason you need to print here instead of returning an error?
It works fine however the right approach will be to add an error in the return as you mentioned as the return type of the function is error.
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.
return is inside getController := func(ref namespacedOwnerReference) (metav1.Object, error) {
Ah, I see. GitHub doesn't show the indentation correctly.
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 @Miciah ! return is inside getController := func(ref namespacedOwnerReference) (metav1.Object, error) {
so I will add return nil, fmt.Errorf("idling StatefulSets is not supported") as can't return 3 values.
@Miciah after adding this I don't see the error message like I do see with this commit as follows -
[miheer@localhost oc]$ ./oc idle nginx
WARNING: idling when network policies are in place may cause connections to bypass network policy entirely
Idling StatefulSet is not supported yet.
error: unable to mark the service "nginx/nginx" as idled.
Make sure that the service is not already marked as idled and that it is associated with resources that can be scaled.
See 'oc idle -h' for help and examples.
[miheer@localhost oc]$
After adding the return nil, fmt.Errorf("idling StatefulSets is not supported") it does not show the error
[miheer@localhost oc]$ ./oc idle nginx
WARNING: idling when network policies are in place may cause connections to bypass network policy entirely
error: unable to mark the service "nginx/nginx" as idled.
Make sure that the service is not already marked as idled and that it is associated with resources that can be scaled.
See 'oc idle -h' for help and examples.
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.
BTW I had to make the code changes where get controller is called and where it's error is captured if you want to return error. PTAL.
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.
[miheer@localhost oc]$ ./oc idle nginx
WARNING: idling when network policies are in place may cause connections to bypass network policy entirely
error: no valid scalable resources found to idle: unable to calculate scalable resources for service nginx/nginx: Idling StatefulSet is not supported yet.
[miheer@localhost oc]$
251b6db
to
13307d7
Compare
pkg/cli/idle/idle.go
Outdated
if err != nil { | ||
return nil, err | ||
} else if errors.IsNotFound(err) { | ||
return nil, fmt.Errorf("unable to load %s %q: %v", controllerRef.Kind, controllerRef.Name, err) | ||
} |
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.
@miheer, after your change, the else
branch is unreachable, isn't it?
Anyway, it seems the logic was broken by an earlier change: https://github.com/openshift/origin/pull/22919/files#diff-037051bf889204d52030b00c7a2754f939bb8653b22842a3dc53db1259f433d6L420-R420. The original intention was any errors except not-found errors should be reported.
if err != nil { | |
return nil, err | |
} else if errors.IsNotFound(err) { | |
return nil, fmt.Errorf("unable to load %s %q: %v", controllerRef.Kind, controllerRef.Name, err) | |
} | |
if err != nil && !errors.IsNotFound(err) { | |
return nil, fmt.Errorf("unable to load %s %q: %v", controllerRef.Kind, controllerRef.Name, err) | |
} |
@mfojtik, can you double-check that my analysis of your change and suggestion here are correct?
13307d7
to
496aae9
Compare
@soltysh, can you approve this PR? |
pkg/cli/idle/idle.go
Outdated
@@ -268,6 +268,9 @@ func (o *IdleOptions) calculateIdlableAnnotationsByService(infoVisitor func(reso | |||
} | |||
// just get the unversioned version of this | |||
gk := schema.GroupKind{Group: gv.Group, Kind: ref.Kind} | |||
if gk.Kind == "StatefulSet" { | |||
return nil, fmt.Errorf("Idling StatefulSet is not supported yet.") |
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.
All errors should be lowercased, ie. idling StatefulSet...
pkg/cli/idle/idle.go
Outdated
@@ -268,6 +268,9 @@ func (o *IdleOptions) calculateIdlableAnnotationsByService(infoVisitor func(reso | |||
} | |||
// just get the unversioned version of this | |||
gk := schema.GroupKind{Group: gv.Group, Kind: ref.Kind} | |||
if gk.Kind == "StatefulSet" { |
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'd suggest being more accurate ie.:
if kappsv1.SchemeGroupVersion.WithKind("StatefulSet").GroupKind() == gk {
...
this will ensure that only k8s apps StatefulSet will be ignored.
496aae9
to
2db69c3
Compare
Idling a StatefulSet seems to work however accessing the Services Route does not wake up the application ("Application is not available" error page is returned). So, with this fix we will return an error saying that we don't support idling of stateful sets as the accessing the routes for the service which is idled does not work. For that a feature work will be required. But, for now we need to add a error whenever a customer tries to idle a stateful service.
2db69c3
to
0315dde
Compare
/retest |
1 similar comment
/retest |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: miheer, soltysh 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 |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
@miheer: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@miheer: All pull requests linked via external trackers have merged: Bugzilla bug 1976894 has been moved to the MODIFIED state. In response to this:
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. |
/bugzilla refresh |
@Miciah: Bugzilla bug 1976894 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state. In response to this:
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. |
Idling a StatefulSet seems to work however accessing the Services Route does not wake up the application ("Application is not available" error page is returned).
So, with this fix we will return an error saying that we don't support idling of stateful sets as the accessing the routes for the service which is idled does not work. For that a feature work will be required. But, for now we need to add a error whenever a customer tries to idle a stateful service. This commit fixes Bug 1976894.