-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Implementing the storage version API, Part I #84016
Implementing the storage version API, Part I #84016
Conversation
@caesarxuchao: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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/test-infra repository. |
@caesarxuchao: The label(s) In response to this:
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/test-infra repository. |
52ada48
to
e16e33e
Compare
e16e33e
to
4e33274
Compare
bfb5f7e
to
8f896d3
Compare
@caesarxuchao: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
The pull-kubernetes-e2e-gce-alpha-features failed because the alpha API (storageversion) was not enabled. |
To fix pull-kubernetes-e2e-gce-alpha-features, we need to change its job yaml. It only runs some alpha e2e tests. OTOH, the pull-kubernetes-local-e2e ran all e2e conformance tests. It passed with the storage version feature enabled. So I don't plan to fix pull-kubernetes-e2e-gce-alpha-features. |
} | ||
} | ||
|
||
func (s *DefaultManager) addGroupResource(group, resource string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer schema.GroupResource
return | ||
} | ||
klog.V(2).Infof("successfully updated storage version for %v", r.Resource.Name) | ||
s.RemoveResourceInfo(r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we will want to know the full list of ones we handled. Instead, can you mark it as completed.
|
||
mu sync.RWMutex | ||
resources map[*ResourceInfo]struct{} | ||
groupResources map[string]map[string]struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about groupResources map[schema.GroupResource]resourceStatus
type resourceStatus struct{
done bool
lastErr error
resourceInfo ResourceInfo
}
for our future inspection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also eliminate the parallel maps
|
||
// UpdateStorageVersions updates the StorageVersions. If the updates are | ||
// successful, following calls to Completed() returns true. | ||
func (s *DefaultManager) UpdateStorageVersions(loopbackClientConfig *rest.Config, serverID string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kubeAPIServerClientConfig
. On aggregated APIs, loopback means a different thing.
for r := range resources { | ||
r.DecodableVersions = decodableVersions(r.EquivalentResourceMapper, r.Group, r.Resource.Name) | ||
if err := updateStorageVersionFor(sc, serverID, r.Group+"."+r.Resource.Name, r.EncodingVersion, r.DecodableVersions); err != nil { | ||
klog.Fatalf("failed to update storage version for %v", r.Resource.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not Fatal. perhaps utilruntime.HandleError
if err != nil && !apierrors.IsNotFound(err) { | ||
return err | ||
} | ||
if err != nil && apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err != nil
isn't needed
return err | ||
} | ||
|
||
func localUpdateStorageVersion(sv *v1alpha1.StorageVersion, apiserverID, encodingVersion string, decodableVersions []string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please refactor to build a new object to return that can be assigned instead of mutating the input.
metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` | ||
|
||
// Spec is omitted because there is no spec field. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stub in an empty spec please, so the proto looks normal and the object doesn't look too weird.
@caesarxuchao do you have a separate PR with just the API? |
} | ||
|
||
s.StorageVersion.AddResourceInfo(resources...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this happen before we start listening? if so, add a comment that this approach is safe because of this.
apiGroupInfo := apiservicerest.NewRESTStorage(c.GenericConfig.MergedResourceConfig, c.GenericConfig.RESTOptionsGetter) | ||
if err := s.GenericAPIServer.InstallAPIGroup(&apiGroupInfo); err != nil { | ||
rp := apiserverinternalrest.StorageProvider{} | ||
apiGroupInfo, err := rp.NewRESTStorage(c.GenericConfig.MergedResourceConfig, c.GenericConfig.RESTOptionsGetter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected to see this in the kube-apiserver, not in the aggregator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I let the aggregator host it is because the storage version API contains information about all apiservers (kube-apiserver, apiextension apiserver, aggregator apiserver, and custom apiservers). It's analogous to APIService, which is also hosted by the aggrgator. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I let the aggregator host it is because the storage version API contains information about all apiservers (kube-apiserver, apiextension apiserver, aggregator apiserver, and custom apiservers). It's analogous to APIService, which is also hosted by the aggrgator. WDYT?
I think this is closer to the idea of webhook admission configuration. It's provided by the kube-apiserver for consumption and usage by other servers, but aggregation is focused on its single mission
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@caesarxuchao: PR needs rebase. 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/test-infra repository. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/sig-apimachinery
/assign @deads2k @yliaog @lavalamp
/kind feature
KEP
This PR implements:
This PR is already huge, so I'll do the remaining tasks in follow-up PRs. Some major follow-ups are: