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

set-image function is optimized for out-of-place and found it practically unusable forin-place mode #3444

Open
droot opened this issue Aug 6, 2022 · 9 comments
Labels
area/fn-catalog Functions Catalog enhancement New feature or request triaged Issue has been triaged by adding an `area/` label

Comments

@droot
Copy link
Contributor

droot commented Aug 6, 2022

set-image function is optimized for out-of-place and found it practically unusable forin-place mode. With a few tweaks, UX can be enhanced significantly.

I am writing a kpt package for backstage-ui-plugin (aka CaD UI). This package contains one deployment resource (so only one resource that runs a container image). Here is what deployment looks like:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: backstage
  namespace: example
  labels:
    app.kubernetes.io/name: backstage
spec:
  replicas: 1
  template:
    spec:
      containers:
        - name: backstage
          image: gcr.io/kpt-dev/kpt-backstage-plugins/backstage-plugin-cad:v0.1.0
...
...

Note that the image is gcr.io/kpt-dev/kpt-backstage-plugins/backstage-plugin-cad:v0.1.0.

On day0, I create a deployable instance of this package and I am happy with it.

On day 2, This package is updated upstream by the CaD UI team and new image version is available, say v0.2.0. Now, as a user, I want to update the image version.

As per current docs, updating the image version requires me to run following command:

kpt fn eval -i set-image:v0.1.0 -- name=<> --newTag=<>

Recording my friction points:

  1. I initially thought name to be container name because if you look at image is specified in the container section in pod template and name means the container name.
  2. name is expected to be current image name. In order to get the current image, I had to open the deployment.yaml. If I end up opening the file resource file, then might as well edit the image version right there :) so it defeats the whole point of having that function.
  3. The camelCase command line args newTag and newName were not inline with rest of the command line argument format.

So, here is how we can make it nicer to operate in in-place mode:

Thinking from user's perspective, assume they discover or know the tag before hand ( I know there is separate issue about discovering the new version and updating the tag automatically, but that's not the point here). User just want to apply new tag to the images referenced in the package.

  1. If I have just one reference of image in workload types resources (like in my case), user should be able to update the new tag by just running following command:
kpt fn eval -i set-image:v0.1.0 -- tag=<new-tag>
  1. If user has more than one image referenced in workload types, we should use name argument to match the container they want to update the image of. For ex, backstage is the container name here, so command will look like:
# just update the tag for image specified for container name backstage
kpt fn eval set-image:v0.1.0 -- name=backstage tag=<new-tag>
  1. If user wants to update the image and tag for a given container, the command will look like:
kpt fn eval set-image:v0.1.0 -- name=backstage image=<new-image> tag=<new-tag>
  1. We have two edge cases, what if we have two containers with same name or what if container name is not specified ?. For such edge cases, function target selector can be used to narrow down to the KRM object. For example,
kpt fn eval set-image:v0.1.0 --match-name <my-workload-name> -- tag=<new-tag>

Some more notes:

  1. Running fn eval with non-existing image should tell me couldn't find image with this name.
 kpt fn eval -i set-image:v0.1.0 -- name=backstage newTag=v0.2.0
[RUNNING] "gcr.io/kpt-fn/set-image:v0.1.0"
[PASS] "gcr.io/kpt-fn/set-image:v0.1.0" in 1.1s
  Results:
    [info]: no images changed
  
  1. Latest version v0.1.1 seems to be not working correctly.
@droot droot added enhancement New feature or request area/fn-catalog Functions Catalog labels Aug 6, 2022
@droot
Copy link
Contributor Author

droot commented Aug 6, 2022

/cc @ChristopherFry @bgrant0607

@bgrant0607
Copy link
Contributor

The original issue was #2577, which lists some examples of tools that support similar operations.

I would also look at Flux and ArgoCD image updaters.

I would expect that the new tag and/or digest would be discovered by polling a registry, or would be known in a CI pipeline. Either way, once that information is known, something needs to inject to the proper location during CD without human editing.

My current thinking on promotion across environments (#3280) is to update a multi-environment blueprint, then update the deployment package for each environment. That would be similar to kustomize with a remote base and each environment kustomization.yaml pinned to a specific commit or tag.

@bgrant0607
Copy link
Contributor

In the example, is "backstage" the container name or the workload name? I would normally not recommend the container name to match the resource name, as the latter is often customized but the former generally doesn't. Also, in the case of an app with sidecar containers or a generic app blueprint the workload container may have a generic name like "app", "main", "primary", etc.

@droot
Copy link
Contributor Author

droot commented Aug 8, 2022

The original issue was #2577, which lists some examples of tools that support similar operations.

Ack. Will take a quick look.

Either way, once that information is known, something needs to inject to the proper location during CD without human editing.

Yes, that's what I am planning to use set-image for.

My current thinking on promotion across environments (#3280) is to update a multi-environment blueprint, then update the deployment package for each environment. That would be similar to kustomize with a remote base and each environment kustomization.yaml pinned to a specific commit or tag.

Yes, this is the pattern I will follow. Anything I would like to broadcast in all the environments should be done through the upstream multi-environment blueprint (sort of fits the pattern of a bulk operation on the multi-environment blueprint).

In the example, is "backstage" the container name or the workload name? I would normally not recommend the container name to match the resource name, as the latter is often customized but the former generally doesn't. Also, in the case of an app with sidecar containers or a generic app blueprint the workload container may have a generic name like "app", "main", "primary", etc.

My bad, example is using backstage for both the container and workload name [I will update the package and this example to remove the confusion]. They shouldn't be the same. --match-name filter is applied on the resource name (implemented in kpt fn) and I am proposing a --name argument for set-image function.

@yuwenma
Copy link
Contributor

yuwenma commented Aug 8, 2022

I think it's a fair point that set-image should accept Container .name as the matcher besides Container .image.

"4. ...or what if container name is not specified" --> Container .name is mandatory code. "what if we have two containers with same name " I'm not sure about this. But using target selector is a good idea.

Eventually, set-image should be able to update the new 'image' from a variety of resources: I think this requires non KRM resource support(e.g. read image from CI environment or from a CI config), and having fn-config read values from dynamic input resources. Similar requests have been asked by Slack users. The related umbrella issues are #3396 #2528

@bgrant0607
Copy link
Contributor

There's also:
https://kubernetes.io/docs/reference/labels-annotations-taints/#kubectl-kubernetes-io-default-container

@droot
Copy link
Contributor Author

droot commented Aug 8, 2022

There's also: https://kubernetes.io/docs/reference/labels-annotations-taints/#kubectl-kubernetes-io-default-container

Oh..this is interesting. Didn't know such a thing existed. So the default container annotation (if exists) can also be used to select the container in case of ambiguity.

On a different note: kpt pkg analyze can propose editing suggestions such as default-container for a package because most of the real deployment will have more than one container (logging/mesh sidecar etc.) and having a default container annotation can save so much time during the kubectl logs...etc.

@johnbelamaric
Copy link
Contributor

A common use case is also to simply change the registry name. For example, if I use a private registry and push all my approved images there, then I want all image fields to be updated with just the new registry - or perhaps registry+directory, something like:

artifacts.bigco.com/bigco-productA/someimage:v4.2 -> my-registry.example.com/bigco/bigco-productA/someimage:v4.2
artifacts.bigco.com/bigco-productA/anotherimage:v4.2 -> my-registry.example.com/bigco/bigco-productA/anotherimage:v4.2
artifacts.bigco.com/bigco-productB/someimage:v3.1.1 -> my-registry.example.com/bigco/bigco-productB/someimage:v3.1.1

# also support implicit "docker" registry perhaps
nginx/nginx:v1 -> my-registry.example.com/docker/nginx/nginx:v1
coredns/coredns:v1.9.3 -> my-registry.example.com/docker/coredns/coredns/v1.9.3

The idea being that I should be able to change the registry without changing the image and tag.

@yuwenma yuwenma removed their assignment Jan 13, 2023
@yuwenma yuwenma added good first issue Good for newcomers and removed good first issue Good for newcomers labels Jan 13, 2023
@droot droot added the triaged Issue has been triaged by adding an `area/` label label Jan 26, 2023
@droot droot assigned yuwenma and unassigned yuwenma Jan 26, 2023
@droot droot removed the triaged Issue has been triaged by adding an `area/` label label Jan 26, 2023
@yuwenma
Copy link
Contributor

yuwenma commented Jan 26, 2023

This work is partially done, then we realized some more code refactoring is needed to use the new kpt-fn-sdk.

I don't have short term plan to work on it.

@yuwenma yuwenma added the triaged Issue has been triaged by adding an `area/` label label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fn-catalog Functions Catalog enhancement New feature or request triaged Issue has been triaged by adding an `area/` label
Projects
None yet
Development

No branches or pull requests

4 participants