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.

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

MULTIARCH-5058: Implement the node affinity scoring specs in the ClusterPodPlacementConfig #369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pkenchap
Copy link

Task to add the nodeAffinityScoring for ClusterPodPlacemetnConfig API specs can be updated as in the following to host a skeleton of plugins, currently implementing only the nodeAffinityScoring.

Example:

spec:
  plugins:
    nodeAffinityScoring:
        enabled: true
        platforms:
            - architecture: amd64
              # os: linux # TODO: this structure can support the os field in the future (e.g., for win containers)
              weight: 100

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 20, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 20, 2024

@pkenchap: This pull request references MULTIARCH-5058 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Task to add the nodeAffinityScoring for ClusterPodPlacemetnConfig API specs can be updated as in the following to host a skeleton of plugins, currently implementing only the nodeAffinityScoring.

Example:

spec:
 plugins:
   nodeAffinityScoring:
       enabled: true
       platforms:
           - architecture: amd64
             # os: linux # TODO: this structure can support the os field in the future (e.g., for win containers)
             weight: 100

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 openshift-eng/jira-lifecycle-plugin repository.

@pkenchap
Copy link
Author

/assign @pkenchap

@pkenchap pkenchap force-pushed the MULTIARCH-5058 branch 2 times, most recently from 2337fb3 to 7994305 Compare November 21, 2024 08:55
@@ -0,0 +1,5 @@
## Append samples you want in your CSV to this file as resources ##
resources:
- multiarch_v1beta1_nodeaffinityscoring_clusterpodplacementconfig.yaml
Copy link

Choose a reason for hiding this comment

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

I don't think you need this in a subfolder

Copy link

Choose a reason for hiding this comment

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

I'm still not getting the point of this customization.... is it autogenerated?

Copy link
Member

Choose a reason for hiding this comment

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

+1, we do not need this example.

Copy link
Author

Choose a reason for hiding this comment

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

ok sure , I can remove this .

Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this and solve this comment to start wrapping up?

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
plugin := &plugins.BasePlugin{Enabled: tt.enabled}
if plugin.IsEnabled() != tt.expected {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if plugin.IsEnabled() != tt.expected {
if plugin.IsEnabled() != tt.enabled {

This unit test concers the correcteness of the IsEnabled function. Given the simplicity, it's ok not to duplicate the expected value and use tt.enabled.

Comment on lines 41 to 46

// Plugins selects the namespaces where the pod placement operand can process the nodeAffinityScoring
// of the pods. If left empty, all the namespaces are considered.
// The default sample allows to exclude all the namespaces where the
// +optional
Plugins plugins.Plugins `json:"plugins,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Plugins selects the namespaces where the pod placement operand can process the nodeAffinityScoring
// of the pods. If left empty, all the namespaces are considered.
// The default sample allows to exclude all the namespaces where the
// +optional
Plugins plugins.Plugins `json:"plugins,omitempty"`

We should not update the v1alpha1 api.

@pkenchap pkenchap force-pushed the MULTIARCH-5058 branch 5 times, most recently from 3c1b3d3 to 66961d9 Compare February 6, 2025 07:58
@pkenchap
Copy link
Author

pkenchap commented Feb 6, 2025

/retest

Comment on lines 593 to 626
By("Creating a v1beta1 ClusterPodPlacementConfig")
// err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).Build())
// Expect(err).NotTo(HaveOccurred(), "failed to create ClusterPodPlacementConfig", err)
// validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err := k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
Spec: v1beta1.ClusterPodPlacementConfigSpec{
LogVerbosity: "Normal",
Plugins: plugins.Plugins{
NodeAffinityScoring: &plugins.NodeAffinityScoring{
BasePlugin: plugins.BasePlugin{
Enabled: true,
},
Platforms: []plugins.NodeAffinityScoringPlatformTerm{
{Architecture: utils.ArchitectureArm64, Weight: 50},
},
},
},
},
}), ppc)
Expect(err).NotTo(HaveOccurred())
Eventually(func(g Gomega) {
// Verify the LogVerbosity field
g.Expect(ppc.Spec.LogVerbosity).NotTo(Equal("Normal"))
// Verify the Plugins field
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring).To(BeNil())
}).Should(Succeed())
})
It("does not query the remote registry until the cache expires", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By("Creating a v1beta1 ClusterPodPlacementConfig")
// err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).Build())
// Expect(err).NotTo(HaveOccurred(), "failed to create ClusterPodPlacementConfig", err)
// validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err := k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
Spec: v1beta1.ClusterPodPlacementConfigSpec{
LogVerbosity: "Normal",
Plugins: plugins.Plugins{
NodeAffinityScoring: &plugins.NodeAffinityScoring{
BasePlugin: plugins.BasePlugin{
Enabled: true,
},
Platforms: []plugins.NodeAffinityScoringPlatformTerm{
{Architecture: utils.ArchitectureArm64, Weight: 50},
},
},
},
},
}), ppc)
Expect(err).NotTo(HaveOccurred())
Eventually(func(g Gomega) {
// Verify the LogVerbosity field
g.Expect(ppc.Spec.LogVerbosity).NotTo(Equal("Normal"))
// Verify the Plugins field
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring).To(BeNil())
}).Should(Succeed())
})
It("does not query the remote registry until the cache expires", func() {

}).Should(Succeed())
})
It("does not query the remote registry until the cache expires", func() {
cppc := &v1beta1.ClusterPodPlacementConfig{ObjectMeta: metav1.ObjectMeta{
Copy link
Member

Choose a reason for hiding this comment

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

We should use the builder here.

Suggested change
cppc := &v1beta1.ClusterPodPlacementConfig{ObjectMeta: metav1.ObjectMeta{
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(true).WithNodeAffinityScoringTerm(utils.ArchitectureArm64, 50).Build())
Expect(err).NotTo(HaveOccurred(), "failed to create ClusterPodPlacementConfig", err)
validateReconcile()

Where WithNodeAffinityScoringPlugin and WithNodeAffinityScoringTerm are two methods to be implemented in https://github.com/openshift/multiarch-tuning-operator/blob/main/pkg/testing/builder/clusterpodplacementconfig.go.

The first only gets the value for the enabled field, the second gets (string, int) to append an element in the Platforms slice of the Plugins.NodeAffinityScoring spec of the CPPC.

Comment on lines 651 to 619
}, v1alpha1obj)
Eventually(func(g Gomega) {
// Verify the LogVerbosity field
g.Expect(v1alpha1obj.Spec.LogVerbosity).NotTo(Equal("Normal"))

// Then, fetch it to ensure it exists and check the fields
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(cppc), ppc)
Expect(err).NotTo(HaveOccurred())

// Verify the LogVerbosity field
g.Expect(ppc.Spec.LogVerbosity).To(Equal("Normal"))
// Verify the Plugins field
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring).NotTo(BeNil())
// verify the enabled bool
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring.Enabled).To(BeTrue())
// verify platform
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring.Platforms).To(ConsistOf(
plugins.NodeAffinityScoringPlatformTerm{Architecture: "ppc64le", Weight: 50},
))
}).Should(Succeed())
})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, v1alpha1obj)
Eventually(func(g Gomega) {
// Verify the LogVerbosity field
g.Expect(v1alpha1obj.Spec.LogVerbosity).NotTo(Equal("Normal"))
// Then, fetch it to ensure it exists and check the fields
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(cppc), ppc)
Expect(err).NotTo(HaveOccurred())
// Verify the LogVerbosity field
g.Expect(ppc.Spec.LogVerbosity).To(Equal("Normal"))
// Verify the Plugins field
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring).NotTo(BeNil())
// verify the enabled bool
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring.Enabled).To(BeTrue())
// verify platform
g.Expect(ppc.Spec.Plugins.NodeAffinityScoring.Platforms).To(ConsistOf(
plugins.NodeAffinityScoringPlatformTerm{Architecture: "ppc64le", Weight: 50},
))
}).Should(Succeed())
})
}, v1alpha1obj)
Expect(err).NotTo(HaveOccurred())

No need for the Eventually block here.

@pkenchap pkenchap force-pushed the MULTIARCH-5058 branch 3 times, most recently from 4e57349 to 1188b97 Compare February 6, 2025 13:42
@pkenchap
Copy link
Author

pkenchap commented Feb 6, 2025

/test unit

}

func (p *ClusterPodPlacementConfigBuilder) WithNodeAffinityScoringTerm(architecture string, weight int32) *ClusterPodPlacementConfigBuilder {
p.Spec.Plugins.NodeAffinityScoring.Platforms = []plugins.NodeAffinityScoringPlatformTerm{
Copy link
Member

Choose a reason for hiding this comment

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

We should append to an existing Platforms list or create if it's still nil

Copy link
Author

Choose a reason for hiding this comment

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

yes , even test was failing as it was not initialised . will check for nil.

@@ -34,3 +35,14 @@ func (p *ClusterPodPlacementConfigBuilder) WithLogVerbosity(logVerbosity common.
func (p *ClusterPodPlacementConfigBuilder) Build() *v1beta1.ClusterPodPlacementConfig {
return p.ClusterPodPlacementConfig
}

func (p *ClusterPodPlacementConfigBuilder) WithNodeAffinityScoring(enabled bool) *ClusterPodPlacementConfigBuilder {
p.Spec.Plugins.NodeAffinityScoring.Enabled = enabled
Copy link
Member

Choose a reason for hiding this comment

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

You'd check if Plugins and NodeAffinityScoring are initialized first

Copy link
Author

Choose a reason for hiding this comment

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

yes sure , updated .

@pkenchap pkenchap force-pushed the MULTIARCH-5058 branch 2 times, most recently from 034c721 to 63bda32 Compare February 7, 2025 04:33
@pkenchap
Copy link
Author

pkenchap commented Feb 7, 2025

/retest-required

Comment on lines 620 to 601
It("Should be possible to get the CPPC as a v1alpha1 with specs invalid weight regarding the plugins", func() {
By("Creating a v1beta1 ClusterPodPlacementConfig invalid weight")
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(true).WithNodeAffinityScoringTerm(utils.ArchitectureArm64, 200).Build())
Expect(err).NotTo(HaveOccurred(), "failed to create v1beta1 ClusterPodPlacementConfig invalid weight", err)
validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
}), ppc)
Expect(err).NotTo(HaveOccurred(), "failed to get v1beta1 ClusterPodPlacementConfig invalid weight", err)
// Get v1alpha1 ClusterPodPlacementConfig
By("Get a v1alpha1 ClusterPodPlacementConfig")
v1alpha1obj := &v1alpha1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKey{
Name: common.SingletonResourceObjectName,
}, v1alpha1obj)
Expect(err).NotTo(HaveOccurred(), "failed to get v1alpha1 ClusterPodPlacementConfig invalid weight", err)
})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It("Should be possible to get the CPPC as a v1alpha1 with specs invalid weight regarding the plugins", func() {
By("Creating a v1beta1 ClusterPodPlacementConfig invalid weight")
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(true).WithNodeAffinityScoringTerm(utils.ArchitectureArm64, 200).Build())
Expect(err).NotTo(HaveOccurred(), "failed to create v1beta1 ClusterPodPlacementConfig invalid weight", err)
validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
}), ppc)
Expect(err).NotTo(HaveOccurred(), "failed to get v1beta1 ClusterPodPlacementConfig invalid weight", err)
// Get v1alpha1 ClusterPodPlacementConfig
By("Get a v1alpha1 ClusterPodPlacementConfig")
v1alpha1obj := &v1alpha1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKey{
Name: common.SingletonResourceObjectName,
}, v1alpha1obj)
Expect(err).NotTo(HaveOccurred(), "failed to get v1alpha1 ClusterPodPlacementConfig invalid weight", err)
})
It("Should not allow invalid weigths", func() {
By("Creating a v1beta1 ClusterPodPlacementConfig invalid weight")
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(true).WithNodeAffinityScoringTerm(utils.ArchitectureArm64, 200).Build())
Expect(err).To(HaveOccurred(), "no ClusterPodPlacementConfig with invalid weights in the plugins.NodeAffinityScoring.Platforms[].Weight should be accepted", err)
})

Comment on lines 642 to 662
It("Should be possible to get the CPPC as a v1alpha1 with specs only enabled regarding the plugins", func() {
By("Creating a v1beta1 ClusterPodPlacementConfig invalid weight")
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(true).Build())
Expect(err).NotTo(HaveOccurred(), "failed to create v1beta1 ClusterPodPlacementConfig enabled", err)
validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
}), ppc)
Expect(err).NotTo(HaveOccurred(), "failed to get v1beta1 ClusterPodPlacementConfig enabled", err)
// Get v1alpha1 ClusterPodPlacementConfig
By("Get a v1alpha1 ClusterPodPlacementConfig")
v1alpha1obj := &v1alpha1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKey{
Name: common.SingletonResourceObjectName,
}, v1alpha1obj)
Expect(err).NotTo(HaveOccurred(), "failed to get v1alpha1 ClusterPodPlacementConfig enabled", err)
})
It("Should be possible to get the CPPC as a v1alpha1 with specs enabled false regarding the plugins", func() {
By("Creating a v1beta1 ClusterPodPlacementConfig invalid weight")
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(false).Build())
Expect(err).NotTo(HaveOccurred(), "failed to create v1beta1 ClusterPodPlacementConfig enabled false", err)
validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
}), ppc)
Expect(err).NotTo(HaveOccurred(), "failed to get v1beta1 ClusterPodPlacementConfig enabled false", err)
// Get v1alpha1 ClusterPodPlacementConfig
By("Get a v1alpha1 ClusterPodPlacementConfig")
v1alpha1obj := &v1alpha1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKey{
Name: common.SingletonResourceObjectName,
}, v1alpha1obj)
Expect(err).NotTo(HaveOccurred(), "failed to get v1alpha1 ClusterPodPlacementConfig enabled false", err)
})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It("Should be possible to get the CPPC as a v1alpha1 with specs only enabled regarding the plugins", func() {
By("Creating a v1beta1 ClusterPodPlacementConfig invalid weight")
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(true).Build())
Expect(err).NotTo(HaveOccurred(), "failed to create v1beta1 ClusterPodPlacementConfig enabled", err)
validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
}), ppc)
Expect(err).NotTo(HaveOccurred(), "failed to get v1beta1 ClusterPodPlacementConfig enabled", err)
// Get v1alpha1 ClusterPodPlacementConfig
By("Get a v1alpha1 ClusterPodPlacementConfig")
v1alpha1obj := &v1alpha1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKey{
Name: common.SingletonResourceObjectName,
}, v1alpha1obj)
Expect(err).NotTo(HaveOccurred(), "failed to get v1alpha1 ClusterPodPlacementConfig enabled", err)
})
It("Should be possible to get the CPPC as a v1alpha1 with specs enabled false regarding the plugins", func() {
By("Creating a v1beta1 ClusterPodPlacementConfig invalid weight")
err := k8sClient.Create(ctx, builder.NewClusterPodPlacementConfig().WithName(common.SingletonResourceObjectName).WithNodeAffinityScoring(false).Build())
Expect(err).NotTo(HaveOccurred(), "failed to create v1beta1 ClusterPodPlacementConfig enabled false", err)
validateReconcile()
// Get the details
ppc := &v1beta1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKeyFromObject(&v1beta1.ClusterPodPlacementConfig{
ObjectMeta: metav1.ObjectMeta{
Name: common.SingletonResourceObjectName,
Namespace: utils.Namespace(),
},
}), ppc)
Expect(err).NotTo(HaveOccurred(), "failed to get v1beta1 ClusterPodPlacementConfig enabled false", err)
// Get v1alpha1 ClusterPodPlacementConfig
By("Get a v1alpha1 ClusterPodPlacementConfig")
v1alpha1obj := &v1alpha1.ClusterPodPlacementConfig{}
err = k8sClient.Get(ctx, crclient.ObjectKey{
Name: common.SingletonResourceObjectName,
}, v1alpha1obj)
Expect(err).NotTo(HaveOccurred(), "failed to get v1alpha1 ClusterPodPlacementConfig enabled false", err)
})

@pkenchap pkenchap force-pushed the MULTIARCH-5058 branch 4 times, most recently from 2b1266b to 42fcd8d Compare February 10, 2025 05:31
…terPodPlacementConfig

Signed-off-by: Punith Kenchappa <[email protected]>
Copy link

openshift-ci bot commented Feb 10, 2025

@pkenchap: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/unit 42fcd8d link true /test unit

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants