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

Porch: Move Resource Version Annotation to different file #4017

Closed
bharat-m1 opened this issue Aug 3, 2023 · 4 comments
Closed

Porch: Move Resource Version Annotation to different file #4017

bharat-m1 opened this issue Aug 3, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@bharat-m1
Copy link
Contributor

As part of our upgrade process to v0.0.20, we observed that a new field resource-version annotation is added to Kptfile which captures the source commit id to handle concurrency issues.

Before this change all the attributes in the Kptfile were Package level attributes and now we have introduced PackageRevision specific attribute which is only available when we pull the PackageRevision. 

When using the previous version we used to copy all the files from our base package which include the Kptfile and modify the setters file as per our need. With the latest change we cannot copy the Kptfile directly as that will override the resource-version annotation and throw an error while pushing the PackageRevision. Hence it is changing the previous experience we have and introducing some more steps in between.

So can we store the resource-version into a different file (say KptRevisionFile), so that we can have the same experience.

@bharat-m1 bharat-m1 added the enhancement New feature or request label Aug 3, 2023
@bharat-m1 bharat-m1 changed the title Porch: Upgrading to Porch 0.20 Porch: Move Resource Version Annotation to different file Aug 3, 2023
@johnbelamaric
Copy link
Contributor

johnbelamaric commented Aug 3, 2023

That could be OK. One question though - could you use kpt pkg get https://.../myrepo.git@mypackage/vX instead? That will not add the annotation.

When using the Porch APIs, resource version is just the standard metadata field in the resource. When we do a kpt alpha rpkg pull, we are only grabbing the Resources and writing them to disk, so that metadata field would be lost. However, in order to write changes back to the API server, we need to present the resource version that we based the pull on; that way, the API server can reject the push if someone else already modified the resource since we pulled it (this is how k8s manages optimistic concurrency).

In order to fix this, kpt alpha rpkg pull copies the metadata.resourceVersion from the resource and adds it to the Kptfile as an annotation. kpt alpha rpkg push copies it back to metadata.resourceVersion, and removes the annotation. So, the actual Porch server never sees those annotations, it's all done in the client.

The annotation method was just chosen as a convenience; we could instead put it in a .kpt/metadata.yaml or something - this is all just client-side decisions. Of course that would conflict if someone had a file with that name in their package, but I think that's a risk we could take. It may be useful to have some other metadata fields in time, too. Curious about @mortent's thoughts on that.

Your contributions are welcome here. As mentioned, you would just need to change the kpt alpha rpkg pull and push commands. You probably would need to continue to support the annotation on push (but not pull), with the new file taking precedence. A few other potentially useful features along those lines:

  • a push --force option could read the current resource version and forcibly overwrite the server-side with whatever is in the client side. This obviously would be dangerous as it could lose other users edits, but it may be useful occasionally.
  • a pull --update option that did a rebase of your local directory based upon the updates other users have made would be nice. This could be fraught with some real difficulties though.

@mortent
Copy link
Contributor

mortent commented Aug 4, 2023

I agree with @johnbelamaric here. If using kpt pkg get is an option, that may be simpler. But since the annotation is purely used client-side by the pull and push commands, I don't see any major issues with putting the information in a separate file. We could also potentially add a flag on the pull command that causes kpt to not add the annotation, but I'm not convinced about the UX about that approach.

@bharat-m1
Copy link
Contributor Author

@mortent / @johnbelamaric I would like to take up this change and move the annotation to a different file say KptRevisionMetaData which we can use further to add any revision specific data. Let me know your thoughts / concerns

bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 8, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 9, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 9, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 9, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 10, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 11, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 12, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
bharat-m1 added a commit to bharat-m1/kpt that referenced this issue Aug 12, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
johnbelamaric pushed a commit that referenced this issue Aug 12, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: #4017
@bharat-m1
Copy link
Contributor Author

Pr is merged, so closing the issue.

johnbelamaric pushed a commit to mortent/kpt that referenced this issue Sep 18, 2023
The newly added file can be used for storing all the metadata going
forward
It is added and removed from the client side and will not be present in
porch

Please refer this for more info: kptdev#4017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants