Skip to content

Commit

Permalink
Merge pull request kubernetes#6670 from yaroslava-serdiuk/provreqwrapper
Browse files Browse the repository at this point in the history
Cleanup ProvReq wrapper
  • Loading branch information
k8s-ci-robot authored Apr 15, 2024
2 parents 59882e0 + 7ff9480 commit b5cd900
Show file tree
Hide file tree
Showing 15 changed files with 73 additions and 127 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (p *ProvisioningRequestPodsInjector) Process(
return nil, err
}
for _, pr := range provReqs {
conditions := pr.Conditions()
conditions := pr.Status.Conditions
if apimeta.IsStatusConditionTrue(conditions, v1beta1.Failed) || apimeta.IsStatusConditionTrue(conditions, v1beta1.Provisioned) {
continue
}
Expand All @@ -76,7 +76,7 @@ func (p *ProvisioningRequestPodsInjector) Process(
if inject {
provreqpods, err := provreqpods.PodsForProvisioningRequest(pr)
if err != nil {
klog.Errorf("Failed to get pods for ProvisioningRequest %v", pr.Name())
klog.Errorf("Failed to get pods for ProvisioningRequest %v", pr.Name)
provreqconditions.AddOrUpdateCondition(pr, v1beta1.Failed, metav1.ConditionTrue, provreqconditions.FailedToCreatePodsReason, err.Error(), metav1.NewTime(p.clock.Now()))
continue
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,19 @@ func TestProvisioningRequestPodsInjector(t *testing.T) {
name: "New ProvisioningRequest, pods are injected and Accepted condition is added",
provReqs: []*provreqwrapper.ProvisioningRequest{newProvReqA, provisionedAcceptedProvReqB},
wantUnscheduledPodCount: podsA,
wantUpdatedConditionName: newProvReqA.Name(),
wantUpdatedConditionName: newProvReqA.Name,
},
{
name: "New ProvisioningRequest, pods are injected and Accepted condition is updated",
provReqs: []*provreqwrapper.ProvisioningRequest{newAcceptedProvReqA, provisionedAcceptedProvReqB},
wantUnscheduledPodCount: podsA,
wantUpdatedConditionName: newAcceptedProvReqA.Name(),
wantUpdatedConditionName: newAcceptedProvReqA.Name,
},
{
name: "Provisioned=False, pods are injected",
provReqs: []*provreqwrapper.ProvisioningRequest{notProvisionedAcceptedProvReqB, failedProvReq},
wantUnscheduledPodCount: podsB,
wantUpdatedConditionName: notProvisionedAcceptedProvReqB.Name(),
wantUpdatedConditionName: notProvisionedAcceptedProvReqB.Name,
},
{
name: "Provisioned=True, no pods are injected",
Expand All @@ -124,7 +124,7 @@ func TestProvisioningRequestPodsInjector(t *testing.T) {
continue
}
pr, _ := client.ProvisioningRequest("ns", tc.wantUpdatedConditionName)
accepted := apimeta.FindStatusCondition(pr.Conditions(), v1beta1.Accepted)
accepted := apimeta.FindStatusCondition(pr.Status.Conditions, v1beta1.Accepted)
if accepted == nil || accepted.LastTransitionTime != metav1.NewTime(now) {
t.Errorf("%s: injector.Process hasn't update accepted condition for ProvisioningRequest %s", tc.name, tc.wantUpdatedConditionName)
}
Expand All @@ -134,6 +134,6 @@ func TestProvisioningRequestPodsInjector(t *testing.T) {

func testProvisioningRequestWithCondition(name string, podCount int, conditions ...metav1.Condition) *provreqwrapper.ProvisioningRequest {
pr := provreqwrapper.BuildTestProvisioningRequest("ns", name, "10", "100", "", int32(podCount), false, time.Now(), "ProvisioningClass")
pr.V1Beta1().Status.Conditions = conditions
pr.Status.Conditions = conditions
return pr
}
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func (o *provReqOrchestrator) verifyProvisioningRequestClass(unschedulablePods [
if err != nil {
return nil, fmt.Errorf("Failed retrive ProvisioningRequest from unscheduled pods, err: %v", err)
}
if provReq.V1Beta1().Spec.ProvisioningClassName != v1beta1.ProvisioningClassCheckCapacity {
if provReq.Spec.ProvisioningClassName != v1beta1.ProvisioningClassCheckCapacity {
return nil, fmt.Errorf("ProvisioningRequestClass is not %s", v1beta1.ProvisioningClassCheckCapacity)
}
for _, pod := range unschedulablePods {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ func (p *checkCapacityProcessor) Process(provReqs []*provreqwrapper.Provisioning
if len(expiredProvReq) >= p.maxUpdated {
break
}
conditions := provReq.Conditions()
if provReq.V1Beta1().Spec.ProvisioningClassName != v1beta1.ProvisioningClassCheckCapacity ||
conditions := provReq.Status.Conditions
if provReq.Spec.ProvisioningClassName != v1beta1.ProvisioningClassCheckCapacity ||
apimeta.IsStatusConditionTrue(conditions, v1beta1.BookingExpired) || apimeta.IsStatusConditionTrue(conditions, v1beta1.Failed) {
continue
}
Expand All @@ -65,7 +65,7 @@ func (p *checkCapacityProcessor) Process(provReqs []*provreqwrapper.Provisioning
expiredProvReq = append(expiredProvReq, provReq)
}
} else if len(failedProvReq) < p.maxUpdated-len(expiredProvReq) {
created := provReq.CreationTimestamp()
created := provReq.CreationTimestamp
if created.Add(defaultExpirationTime).Before(p.now()) {
failedProvReq = append(failedProvReq, provReq)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,15 +140,15 @@ func TestProcess(t *testing.T) {
}
for _, test := range testCases {
pr := provreqclient.ProvisioningRequestWrapperForTesting("namespace", "name-1")
pr.V1Beta1().Status.Conditions = test.conditions
pr.V1Beta1().CreationTimestamp = metav1.NewTime(test.creationTime)
pr.V1Beta1().Spec.ProvisioningClassName = v1beta1.ProvisioningClassCheckCapacity
pr.Status.Conditions = test.conditions
pr.CreationTimestamp = metav1.NewTime(test.creationTime)
pr.Spec.ProvisioningClassName = v1beta1.ProvisioningClassCheckCapacity
additionalPr := provreqclient.ProvisioningRequestWrapperForTesting("namespace", "additional")
additionalPr.V1Beta1().CreationTimestamp = metav1.NewTime(weekAgo)
additionalPr.V1Beta1().Spec.ProvisioningClassName = v1beta1.ProvisioningClassCheckCapacity
additionalPr.CreationTimestamp = metav1.NewTime(weekAgo)
additionalPr.Spec.ProvisioningClassName = v1beta1.ProvisioningClassCheckCapacity
processor := checkCapacityProcessor{func() time.Time { return now }, 1}
processor.Process([]*provreqwrapper.ProvisioningRequest{pr, additionalPr})
assert.ElementsMatch(t, test.wantConditions, pr.Conditions())
assert.ElementsMatch(t, test.wantConditions, pr.Status.Conditions)
if len(test.conditions) == len(test.wantConditions) {
assert.ElementsMatch(t, []metav1.Condition{
{
Expand All @@ -158,9 +158,9 @@ func TestProcess(t *testing.T) {
Reason: conditions.ExpiredReason,
Message: conditions.ExpiredMsg,
},
}, additionalPr.Conditions())
}, additionalPr.Status.Conditions)
} else {
assert.ElementsMatch(t, []metav1.Condition{}, additionalPr.Conditions())
assert.ElementsMatch(t, []metav1.Condition{}, additionalPr.Status.Conditions)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func TestBookCapacity(t *testing.T) {
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pr := provreqwrapper.NewV1Beta1ProvisioningRequest(
pr := provreqwrapper.NewProvisioningRequest(
&v1beta1.ProvisioningRequest{
Spec: v1beta1.ProvisioningRequestSpec{
ProvisioningClassName: v1beta1.ProvisioningClassCheckCapacity,
Expand Down Expand Up @@ -247,14 +247,14 @@ func TestSetCondition(t *testing.T) {
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pr := provreqwrapper.NewV1Beta1ProvisioningRequest(
pr := provreqwrapper.NewProvisioningRequest(
&v1beta1.ProvisioningRequest{
Status: v1beta1.ProvisioningRequestStatus{
Conditions: test.oldConditions,
},
}, nil)
AddOrUpdateCondition(pr, test.newType, test.newStatus, "", "", v1.Now())
got := pr.Conditions()
got := pr.Status.Conditions
if len(got) > 2 || len(got) != len(test.want) || got[0].Type != test.want[0].Type || got[0].Status != test.want[0].Status {
t.Errorf("want %v, got: %v", test.want, got)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ const (

// ShouldCapacityBeBooked returns whether capacity should be booked.
func ShouldCapacityBeBooked(pr *provreqwrapper.ProvisioningRequest) bool {
if pr.V1Beta1().Spec.ProvisioningClassName != v1beta1.ProvisioningClassCheckCapacity {
if pr.Spec.ProvisioningClassName != v1beta1.ProvisioningClassCheckCapacity {
return false
}
conditions := pr.Conditions()
conditions := pr.Status.Conditions
if apimeta.IsStatusConditionTrue(conditions, v1beta1.Failed) || apimeta.IsStatusConditionTrue(conditions, v1beta1.BookingExpired) {
return false
} else if apimeta.IsStatusConditionTrue(conditions, v1beta1.Provisioned) {
Expand All @@ -69,12 +69,12 @@ func AddOrUpdateCondition(pr *provreqwrapper.ProvisioningRequest, conditionType
newCondition := metav1.Condition{
Type: conditionType,
Status: conditionStatus,
ObservedGeneration: pr.V1Beta1().GetObjectMeta().GetGeneration(),
ObservedGeneration: pr.GetObjectMeta().GetGeneration(),
LastTransitionTime: now,
Reason: reason,
Message: message,
}
prevConditions := pr.Conditions()
prevConditions := pr.Status.Conditions
switch conditionType {
case v1beta1.Provisioned, v1beta1.BookingExpired, v1beta1.Failed, v1beta1.Accepted:
conditionFound := false
Expand Down
20 changes: 10 additions & 10 deletions cluster-autoscaler/provisioningrequest/pods/pods.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ func PodsForProvisioningRequest(pr *provreqwrapper.ProvisioningRequest) ([]*v1.P
pods := make([]*v1.Pod, 0)
for i, podSet := range podSets {
for j := 0; j < int(podSet.Count); j++ {
pod, err := controller.GetPodFromTemplate(&podSet.PodTemplate, pr.RuntimeObject(), ownerReference(pr))
pod, err := controller.GetPodFromTemplate(&podSet.PodTemplate, pr.ProvisioningRequest, ownerReference(pr))
if err != nil {
return nil, fmt.Errorf("while creating pod for pr: %s/%s podSet: %d, got error: %w", pr.Namespace(), pr.Name(), i, err)
return nil, fmt.Errorf("while creating pod for pr: %s/%s podSet: %d, got error: %w", pr.Namespace, pr.Name, i, err)
}
populatePodFields(pr, pod, i, j)
pods = append(pods, pod)
Expand All @@ -63,22 +63,22 @@ func PodsForProvisioningRequest(pr *provreqwrapper.ProvisioningRequest) ([]*v1.P
// the scale-up simulation logic and number of logs lines emitted.
func ownerReference(pr *provreqwrapper.ProvisioningRequest) *metav1.OwnerReference {
return &metav1.OwnerReference{
APIVersion: pr.APIVersion(),
Kind: pr.Kind(),
Name: pr.Name(),
UID: pr.UID(),
APIVersion: pr.APIVersion,
Kind: pr.Kind,
Name: pr.Name,
UID: pr.UID,
Controller: proto.Bool(true),
}
}

func populatePodFields(pr *provreqwrapper.ProvisioningRequest, pod *v1.Pod, i, j int) {
pod.Name = fmt.Sprintf("%s%d-%d", pod.GenerateName, i, j)
pod.Namespace = pr.Namespace()
pod.Namespace = pr.Namespace
if pod.Annotations == nil {
pod.Annotations = make(map[string]string)
}
pod.Annotations[ProvisioningRequestPodAnnotationKey] = pr.Name()
pod.Annotations[ProvisioningClassPodAnnotationKey] = pr.V1Beta1().Spec.ProvisioningClassName
pod.Annotations[ProvisioningRequestPodAnnotationKey] = pr.Name
pod.Annotations[ProvisioningClassPodAnnotationKey] = pr.Spec.ProvisioningClassName
pod.UID = types.UID(fmt.Sprintf("%s/%s", pod.Namespace, pod.Name))
pod.CreationTimestamp = pr.CreationTimestamp()
pod.CreationTimestamp = pr.CreationTimestamp
}
3 changes: 1 addition & 2 deletions cluster-autoscaler/provisioningrequest/pods/pods_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/google/go-cmp/cmp"
"google.golang.org/protobuf/proto"

// "google.golang.org/protobuf/testing/protocmp"
apiv1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -256,7 +255,7 @@ func TestPodsForProvisioningRequest(t *testing.T) {
}
for _, tc := range tests {
t.Run(tc.desc, func(t *testing.T) {
got, err := PodsForProvisioningRequest(provreqwrapper.NewV1Beta1ProvisioningRequest(tc.pr, tc.podTemplates))
got, err := PodsForProvisioningRequest(provreqwrapper.NewProvisioningRequest(tc.pr, tc.podTemplates))
if (err != nil) != tc.wantErr {
t.Errorf("PodsForProvisioningRequest() error = %v, wantErr %v", err, tc.wantErr)
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func (c *ProvisioningRequestClient) ProvisioningRequest(namespace, name string)
if err != nil {
return nil, fmt.Errorf("while fetching pod templates for Get Provisioning Request %s/%s got error: %v", namespace, name, err)
}
return provreqwrapper.NewV1Beta1ProvisioningRequest(v1Beta1PR, podTemplates), nil
return provreqwrapper.NewProvisioningRequest(v1Beta1PR, podTemplates), nil
}

// ProvisioningRequests gets all ProvisioningRequest CRs.
Expand All @@ -102,7 +102,7 @@ func (c *ProvisioningRequestClient) ProvisioningRequests() ([]*provreqwrapper.Pr
if errPodTemplates != nil {
return nil, fmt.Errorf("while fetching pod templates for List Provisioning Request %s/%s got error: %v", v1Beta1PR.Namespace, v1Beta1PR.Name, errPodTemplates)
}
prs = append(prs, provreqwrapper.NewV1Beta1ProvisioningRequest(v1Beta1PR, podTemplates))
prs = append(prs, provreqwrapper.NewProvisioningRequest(v1Beta1PR, podTemplates))
}
return prs, nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ func TestFetchPodTemplates(t *testing.T) {

ctx := context.Background()
c := NewFakeProvisioningRequestClient(ctx, t, mockProvisioningRequests...)
got, err := c.FetchPodTemplates(pr1.V1Beta1())
got, err := c.FetchPodTemplates(pr1.ProvisioningRequest)
if err != nil {
t.Errorf("provisioningRequestClient.ProvisioningRequests() error: %v", err)
}
if len(got) != 1 {
t.Errorf("provisioningRequestClient.ProvisioningRequests() got: %v, want 1 element", err)
}
if diff := cmp.Diff(pr1.PodTemplates(), got); diff != "" {
if diff := cmp.Diff(pr1.PodTemplates, got); diff != "" {
t.Errorf("Template mismatch, diff (-want +got):\n%s", diff)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ func NewFakeProvisioningRequestClient(ctx context.Context, t *testing.T, prs ...
if pr == nil {
continue
}
if _, err := provReqClient.AutoscalingV1beta1().ProvisioningRequests(pr.Namespace()).Create(ctx, pr.V1Beta1(), metav1.CreateOptions{}); err != nil {
t.Errorf("While adding a ProvisioningRequest: %s/%s to fake client, got error: %v", pr.Namespace(), pr.Name(), err)
if _, err := provReqClient.AutoscalingV1beta1().ProvisioningRequests(pr.Namespace).Create(ctx, pr.ProvisioningRequest, metav1.CreateOptions{}); err != nil {
t.Errorf("While adding a ProvisioningRequest: %s/%s to fake client, got error: %v", pr.Namespace, pr.Name, err)
}
for _, pd := range pr.PodTemplates() {
if _, err := podTemplClient.CoreV1().PodTemplates(pr.Namespace()).Create(ctx, pd, metav1.CreateOptions{}); err != nil {
t.Errorf("While adding a PodTemplate: %s/%s to fake client, got error: %v", pr.Namespace(), pd.Name, err)
for _, pd := range pr.PodTemplates {
if _, err := podTemplClient.CoreV1().PodTemplates(pr.Namespace).Create(ctx, pd, metav1.CreateOptions{}); err != nil {
t.Errorf("While adding a PodTemplate: %s/%s to fake client, got error: %v", pr.Namespace, pd.Name, err)
}
}
}
Expand Down Expand Up @@ -127,7 +127,7 @@ func ProvisioningRequestWrapperForTesting(namespace, name string) *provreqwrappe
},
}

pr := provreqwrapper.NewV1Beta1ProvisioningRequest(v1Beta1PR, podTemplates)
pr := provreqwrapper.NewProvisioningRequest(v1Beta1PR, podTemplates)
return pr
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func BuildTestProvisioningRequest(namespace, name, cpu, memory, gpu string, podC
},
}
}
return NewV1Beta1ProvisioningRequest(
return NewProvisioningRequest(
&v1beta1.ProvisioningRequest{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand Down
Loading

0 comments on commit b5cd900

Please sign in to comment.