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

[VPA] Add subresource status for vpa with e2e fix #5766

Merged
merged 1 commit into from
Jun 27, 2023
Merged

[VPA] Add subresource status for vpa with e2e fix #5766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion vertical-pod-autoscaler/deploy/vpa-rbac.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ rules:
- get
- list
- watch
- patch
- apiGroups:
- "autoscaling.k8s.io"
resources:
Expand All @@ -53,6 +52,18 @@ rules:
- get
- list
- watch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: system:vpa-status-actor
rules:
- apiGroups:
- "autoscaling.k8s.io"
resources:
- verticalpodautoscalers/status
verbs:
- get
- patch
---
apiVersion: rbac.authorization.k8s.io/v1
Expand Down Expand Up @@ -140,6 +151,19 @@ subjects:
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: system:vpa-status-actor
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: system:vpa-status-actor
subjects:
- kind: ServiceAccount
name: vpa-recommender
namespace: kube-system
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: system:vpa-checkpoint-actor
roleRef:
Expand Down
3 changes: 2 additions & 1 deletion vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,8 @@ spec:
type: object
served: true
storage: true
subresources: {}
subresources:
status: {}
- deprecated: true
deprecationWarning: autoscaling.k8s.io/v1beta2 API is deprecated
name: v1beta2
Expand Down
19 changes: 18 additions & 1 deletion vertical-pod-autoscaler/e2e/v1/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,23 @@ func InstallVPA(f *framework.Framework, vpa *vpa_types.VerticalPodAutoscaler) {
vpaClientSet := getVpaClientSet(f)
_, err := vpaClientSet.AutoscalingV1().VerticalPodAutoscalers(f.Namespace.Name).Create(context.TODO(), vpa, metav1.CreateOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred(), "unexpected error creating VPA")
// apiserver ignore status in vpa create, so need to update status
if !isStatusEmpty(&vpa.Status) {
if vpa.Status.Recommendation != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd write this as one condition:

if !isStatusEmpty(&vpa.Status) && vpa.Status.Recommendation != nil {

PatchVpaRecommendation(f, vpa, vpa.Status.Recommendation)
}
}
}

func isStatusEmpty(status *vpa_types.VerticalPodAutoscalerStatus) bool {
if status == nil {
return true
}

if len(status.Conditions) == 0 && status.Recommendation == nil {
return true
}
return false
}

// InstallRawVPA installs a VPA object passed in as raw json in the test cluster.
Expand All @@ -396,7 +413,7 @@ func PatchVpaRecommendation(f *framework.Framework, vpa *vpa_types.VerticalPodAu
Value: *newStatus,
}})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
_, err = getVpaClientSet(f).AutoscalingV1().VerticalPodAutoscalers(f.Namespace.Name).Patch(context.TODO(), vpa.Name, types.JSONPatchType, bytes, metav1.PatchOptions{})
_, err = getVpaClientSet(f).AutoscalingV1().VerticalPodAutoscalers(f.Namespace.Name).Patch(context.TODO(), vpa.Name, types.JSONPatchType, bytes, metav1.PatchOptions{}, "status")
gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to patch VPA.")
}

Expand Down
19 changes: 18 additions & 1 deletion vertical-pod-autoscaler/e2e/v1beta2/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,23 @@ func InstallVPA(f *framework.Framework, vpa *vpa_types.VerticalPodAutoscaler) {
vpaClientSet := getVpaClientSet(f)
_, err := vpaClientSet.AutoscalingV1beta2().VerticalPodAutoscalers(f.Namespace.Name).Create(context.TODO(), vpa, metav1.CreateOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred(), "unexpected error creating VPA")
// apiserver ignore status in vpa create, so need to update status
if !isStatusEmpty(&vpa.Status) {
if vpa.Status.Recommendation != nil {
PatchVpaRecommendation(f, vpa, vpa.Status.Recommendation)
}
}
wu0407 marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +363 to +368
Copy link
Contributor Author

@wu0407 wu0407 May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A comment is needed in InstallVPA that create vpa with status need individual update status, so isStatusEmpty needs to be preserved, and the code may appear a bit redundant

}

func isStatusEmpty(status *vpa_types.VerticalPodAutoscalerStatus) bool {
if status == nil {
return true
}

if len(status.Conditions) == 0 && status.Recommendation == nil {
return true
}
return false
}

// InstallRawVPA installs a VPA object passed in as raw json in the test cluster.
Expand All @@ -384,7 +401,7 @@ func PatchVpaRecommendation(f *framework.Framework, vpa *vpa_types.VerticalPodAu
Value: *newStatus,
}})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
_, err = getVpaClientSet(f).AutoscalingV1beta2().VerticalPodAutoscalers(f.Namespace.Name).Patch(context.TODO(), vpa.Name, types.JSONPatchType, bytes, metav1.PatchOptions{})
_, err = getVpaClientSet(f).AutoscalingV1beta2().VerticalPodAutoscalers(f.Namespace.Name).Patch(context.TODO(), vpa.Name, types.JSONPatchType, bytes, metav1.PatchOptions{}, "status")
gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to patch VPA.")
}

Expand Down
1 change: 1 addition & 0 deletions vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type VerticalPodAutoscalerList struct {
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:storageversion
// +kubebuilder:resource:shortName=vpa
// +kubebuilder:subresource:status
// +kubebuilder:printcolumn:name="Mode",type="string",JSONPath=".spec.updatePolicy.updateMode"
// +kubebuilder:printcolumn:name="CPU",type="string",JSONPath=".status.recommendation.containerRecommendations[0].target.cpu"
// +kubebuilder:printcolumn:name="Mem",type="string",JSONPath=".status.recommendation.containerRecommendations[0].target.memory"
Expand Down
6 changes: 3 additions & 3 deletions vertical-pod-autoscaler/pkg/utils/vpa/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ type patchRecord struct {
Value interface{} `json:"value"`
}

func patchVpa(vpaClient vpa_api.VerticalPodAutoscalerInterface, vpaName string, patches []patchRecord) (result *vpa_types.VerticalPodAutoscaler, err error) {
func patchVpaStatus(vpaClient vpa_api.VerticalPodAutoscalerInterface, vpaName string, patches []patchRecord) (result *vpa_types.VerticalPodAutoscaler, err error) {
bytes, err := json.Marshal(patches)
if err != nil {
klog.Errorf("Cannot marshal VPA status patches %+v. Reason: %+v", patches, err)
return
}

return vpaClient.Patch(context.TODO(), vpaName, types.JSONPatchType, bytes, meta.PatchOptions{})
return vpaClient.Patch(context.TODO(), vpaName, types.JSONPatchType, bytes, meta.PatchOptions{}, "status")
Copy link
Member

Choose a reason for hiding this comment

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

Is there a unit test we can add to cover this change?
Maybe using the fake client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already have test case on TestUpdateVpaIfNeeded

autoscaler/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go

Lines 93 to 101 in 1384c8b

for _, tc := range testCases {
t.Run(tc.caseName, func(t *testing.T) {
fakeClient := vpa_fake.NewSimpleClientset(&vpa_types.VerticalPodAutoscalerList{Items: []vpa_types.VerticalPodAutoscaler{*tc.observedVpa}})
_, err := UpdateVpaStatusIfNeeded(fakeClient.AutoscalingV1().VerticalPodAutoscalers(tc.updatedVpa.Namespace),
tc.updatedVpa.Name, &tc.updatedVpa.Status, &tc.observedVpa.Status)
assert.NoError(t, err, "Unexpected error occurred.")
actions := fakeClient.Actions()
if tc.expectedUpdate {
assert.Equal(t, 1, len(actions), "Unexpected number of actions")

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, makes sense.

}

// UpdateVpaStatusIfNeeded updates the status field of the VPA API object.
Expand All @@ -69,7 +69,7 @@ func UpdateVpaStatusIfNeeded(vpaClient vpa_api.VerticalPodAutoscalerInterface, v
}}

if !apiequality.Semantic.DeepEqual(*oldStatus, *newStatus) {
return patchVpa(vpaClient, vpaName, patches)
return patchVpaStatus(vpaClient, vpaName, patches)
}
return nil, nil
}
Expand Down