Skip to content

Conversation

@ryanzhang-oss
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss commented Nov 6, 2025

Description of your changes

Add k8s version and cluster api-server to the clusterProfile status by fetching them from the member agent side.

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@ryanzhang-oss ryanzhang-oss force-pushed the add-clusterprofile-status branch 2 times, most recently from 8248438 to 1230ba6 Compare November 6, 2025 03:31
@ryanzhang-oss ryanzhang-oss changed the title Add clusterProfile status feat: add clusterProfile status fields Nov 6, 2025
@ryanzhang-oss ryanzhang-oss force-pushed the add-clusterprofile-status branch from 633304e to ebc35bc Compare November 7, 2025 22:05
Signed-off-by: Ryan Zhang <yangzhangrice@hotmail.com>
Signed-off-by: Ryan Zhang <yangzhangrice@hotmail.com>
@ryanzhang-oss ryanzhang-oss force-pushed the add-clusterprofile-status branch from ebc35bc to f78cdf2 Compare November 7, 2025 22:09
Signed-off-by: Ryan Zhang <yangzhangrice@hotmail.com>
@ryanzhang-oss ryanzhang-oss force-pushed the add-clusterprofile-status branch from 8ca2158 to d0853a5 Compare November 8, 2025 00:19
Signed-off-by: Ryan Zhang <yangzhangrice@hotmail.com>
@ryanzhang-oss ryanzhang-oss force-pushed the add-clusterprofile-status branch from b870cf4 to 1985d16 Compare November 11, 2025 05:11
now := time.Now()

// Check if we have a cached version that is still valid.
p.k8sVersionMutex.RLock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Ryan! Not really a comment or something to change, but it seems that a simple mutex would suffice here :)

Copy link
Collaborator

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

Just a few minor comments, otherwise LGTM.

}

// mockDiscoveryClient is a mock implementation of the discovery.ServerVersionInterface.
type mockDiscoveryClient struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Ryan! Just a nit: discoveryClient seems to have a fake implementation in the client-go package: https://github.com/kubernetes/client-go/blob/d033c497ffef47be9b4f81abde5c3d94dd78089a/discovery/fake/discovery.go#L38.

NodeCountProperty = "kubernetes-fleet.io/node-count"

// K8sVersionProperty is a property that describes the Kubernetes version of the cluster.
K8sVersionProperty = "k8s.io/k8s-version"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Ryan! Just a nit here: I vaguely recall that earlier we were told not to use anything with k8s.io or kubernetes.io in the names; instead all non-Azure properties switched to kubernetes-fleet.io; I don't remember the reasoning behind this though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is designed as an upstream properrty

Signed-off-by: michaelawyu <chenyu1@microsoft.com>
@michaelawyu
Copy link
Collaborator

The work applier IT failure is kind of interesting... The issue is that the system finds an unexpected diff in the .spec.template.metadata.creationTimestamp field ->

This happens because

a) with 0.34 API the creationTimestamp field in the object metadata now has the omitzero field, so in the test code when an object is marshaled in JSON the data won't have this field at all
b) but the K8s environment simulated by envtest uses K8s 1.30, and this version of the API server does not have the omitzero tag enabled for the creationTimestamp field, so when getting an object it will return data that has this field

and as a result when we do the comparison the system will find an extra field and complain about the diff accordingly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants