feat: add clusterProfile status fields #321
Conversation
ryanzhang-oss
force-pushed
the
add-clusterprofile-status
branch
2 times, most recently
from
8248438 to
1230ba6
Compare
ryanzhang-oss
changed the title
ryanzhang-oss
force-pushed
the
add-clusterprofile-status
branch
4 times, most recently
from
b870cf4 to
1985d16
Compare
| now := time.Now() | ||
|
|
||
| // Check if we have a cached version that is still valid. | ||
| p.k8sVersionMutex.RLock() |
There was a problem hiding this comment.
Hi Ryan! Not really a comment or something to change, but it seems that a simple mutex would suffice here :)
There was a problem hiding this comment.
I thought that this function will be called concurrently? Using a reader writer lock can theorically increase the thoroughput?
michaelawyu
left a comment
michaelawyu
left a comment
There was a problem hiding this comment.
Just a few minor comments, otherwise LGTM.
| } | ||
|
|
||
| // mockDiscoveryClient is a mock implementation of the discovery.ServerVersionInterface. | ||
| type mockDiscoveryClient struct { |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
this is designed as an upstream property following the ClusterProperty API KEP
|
The work applier IT failure is kind of interesting... The issue is that the system finds an unexpected diff in the This happens because a) with 0.34 API the and as a result when we do the comparison the system will find an extra field and complain about the diff accordingly |
ryanzhang-oss
force-pushed
the
add-clusterprofile-status
branch
2 times, most recently
from
2dc0544 to
6c434f3
Compare
Signed-off-by: Ryan Zhang <[email protected]>
ryanzhang-oss
force-pushed
the
add-clusterprofile-status
branch
from
6c434f3 to
2a32a1a
Compare
2 participants
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:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer