refactor: image api to create one img that includes list of tags/references #958 #882 #959

jsilverio22 · 2022-12-02T22:35:48Z

#958 #882

Signed-off-by: Joshua Silverio joshua@acorn.io

tylerslaton · 2022-12-05T15:09:43Z

@jsilverio22 Reviewing this now, can you take a look at resolving the merge conflicts?

jsilverio22 · 2022-12-05T15:53:53Z

@jsilverio22 Reviewing this now, can you take a look at resolving the merge conflicts?

should be all up to date now

tylerslaton

Some initial comments that I wanted to get out. Will do some more reviewing later.

pkg/server/registry/images/storage.go

integration/client/images/images_test.go

pkg/cli/images.go

pkg/client/image.go

tylerslaton

Nice work!

pkg/client/image.go

pkg/client/client.go

go.mod

pkg/client/image.go

cjellick · 2022-12-06T20:25:30Z

@jsilverio22 I want to define some behavior/usecases around deleting and untagging images here. Generally, it should follow the behavior of docker rmi, but I want to call out some edge cases explcitly:

When more than one tag exists

Given this list of images:

$ acorn images
REPOSITORY   TAG       IMAGE-ID
image-a      1         ddac371c3e35
image-a      2         ddac371c3e35
image-b      1         ddac371c3e35
image-b      latest    ddac371c3e35

note: image-b:latest can be the result of either acorn build -t image-b . or acorn build -t image-b:latest .

command	expected behavior
`acorn images rm image-a:1`	untag `image-a:1`
`acorn images rm --force image-a:1`	untag `image-a:1`
`acorn images rm image-b`	untag `image-b:latest`
`acorn images rm image-b:latest`	untag `image-b:latest`
`acorn images rm --force ddac371c3e35`	delete the entire image

When exactly one tag exists

Given this list of images:

$ acorn images
REPOSITORY   TAG       IMAGE-ID
image-a      1         ddac371c3e35

command	expected behavior
`acorn images rm image-a:1`	delete the entire image
`acorn images rm ddac371c3e35`	delete the entire image

When the image is "in use" by an app

As described in slack, we may also want to start considering whether there are any apps consuming an image. If we were mimicking docker rmi, we would:

refuse to rm the image unless the user passed --force.
only actually untag the image, but not delete it. it would still be there via digest
update the output of acorn ps to have the digest, rather than the actual image name (though I imagine we'd want to keep app.spec unchanged)

Waiting to hear back from darren on what he'd want to do in those cases. I don't think you should focus on this part yet. I see this "when the image is 'in use' stuff as not critical to the refactor and could be done in a follow up

pkg/apis/api.acorn.io/v1/types.go

tylerslaton

Another round of review. Nothing blocking from my end still, just some food for thought.

pkg/cli/images_rm.go

pkg/client/image.go

pkg/cli/images.go

pkg/tables/tables.go

pkg/client/image.go

pkg/cli/images.go

pkg/server/registry/images/strategy.go

pkg/server/registry/registry.go

pkg/server/registry/images/strategy.go

jsilverio22 · 2022-12-08T16:57:20Z

Next Steps:

Design discussion with @ibuildthecloud on CRDs vs resource version handling vs ignoring conflicts
If Ignore conflicts move tags.Write to handle []strings and remove old tags and write new ones
@cjellick to look into new framework to see if that can remove "do-nothing" methods in API
Move assumption of ":latest" to api side as there are multiple clients who can/will use it
Clarify image print templates (when/where they are used)

pkg/apis/api.acorn.io/v1/types.go

pkg/cli/build.go

pkg/cli/images.go

pkg/tables/tables.go

pkg/cli/images.go

cjellick · 2022-12-10T19:44:27Z

acorn images -c output

If i specify an image (either via tag or digets) as part of the acorn images -c command, but that image has been tagged more than once, I am getting duplicate output.

Given:

# Given this normal image list:
$ acorn images
REPOSITORY   TAG       IMAGE-ID
image-a      1         6123877caafe
image-a      2         6123877caafe

Actual:

 $ acorn images -c image-a:1
REPOSITORY   TAG       IMAGE-ID       CONTAINER   DIGEST
image-a      1         6123877caafe   mycon1      sha256:9fbe5d4f861af0189c42c8eff224f95fe20a2ae91e6ff75f95857cf3bd0c38fa
image-a      2         6123877caafe   mycon1      sha256:9fbe5d4f861af0189c42c8eff224f95fe20a2ae91e6ff75f95857cf3bd0c38fa
image-a      1         6123877caafe   mycon2      sha256:41b16fa94dd2f42e33632e4bfc8fbb11e2dd98f45c3a37e933aba0783e84f8c1
image-a      2         6123877caafe   mycon2      sha256:41b16fa94dd2f42e33632e4bfc8fbb11e2dd98f45c3a37e933aba0783e84f8c1

Expected:

 $ acorn images -c image-a:1
REPOSITORY   TAG       IMAGE-ID       CONTAINER   DIGEST
image-a      1         6123877caafe   mycon1      sha256:9fbe5d4f861af0189c42c8eff224f95fe20a2ae91e6ff75f95857cf3bd0c38fa
image-a      1         6123877caafe   mycon2      sha256:41b16fa94dd2f42e33632e4bfc8fbb11e2dd98f45c3a37e933aba0783e84f8c1

Additionally, If i specify a digest like $ acorn images -c 6123877caafe, it should also similarily limit output to just once....but the old behavior in v0.4.2 is even a little weird...it just picks one of the tags to display. You can probably do the same - just use the first tag if there is one.

pkg/server/registry/images/strategy.go

cjellick · 2022-12-10T22:47:22Z

Tag stopped working since last iteration

acorn tag a:1 b:1
Error: resource name may not be empty

cjellick · 2022-12-10T22:49:44Z

funkiness on kubectl output:

$ acorn images -a
REPOSITORY   TAG       IMAGE-ID
<none>       <none>    a086e2cd2261
a            1         a086e2cd2261
b            1         a086e2cd2261
<none>       <none>    71ad97238b13
simple-demo $ acorn images
REPOSITORY   TAG       IMAGE-ID
a            1         a086e2cd2261
b            1         a086e2cd2261
simple-demo $
simple-demo $
simple-demo $
simple-demo $ kubectl get images -n acorn
IMAGE-ID       TAGS
a086e2cd2261   <none>,a:1,b:1
71ad97238b13   <none>

shouldn't have if theres tags.

cjellick · 2022-12-11T16:25:39Z

integration/client/client.go

@@ -30,5 +31,9 @@ func NewImage(t *testing.T, namespace string) string {
 	if err != nil {
 		t.Fatal(err)
 	}
+	_, err = helper.BuilderClient(t, namespace).ImageCreate(context.Background(), image.ID, "")


when we sync with darren's changes, i dont know if this will stay her permanently or not.

cjellick · 2022-12-11T16:26:39Z

integration/dev/dev_test.go

@@ -59,6 +59,11 @@ func TestDev(t *testing.T) {

 	eg := errgroup.Group{}
 	eg.Go(func() error {
+		_, err = helper.BuilderClient(t, ns.Name).ImageCreate(ctx, "test-app", "")


again, not sure if we will do this permanently. you should keep all these ImageCreate adds in their own single commit that we can nuke later if we need to

cjellick · 2022-12-11T16:33:12Z

Looking good @jsilverio22. You can squash this down now. I think this is as done as it's going to get until we can sync up with Darren's changes.

…rences acorn-io#958 Signed-off-by: Joshua Silverio <joshua@acorn.io>

Signed-off-by: Joshua Silverio <joshua@acorn.io>

ibuildthecloud · 2022-12-16T03:51:40Z

pkg/server/registry/images/tag.go

+			return image, err
+		}
+	}
+	err = t.client.List(ctx, imageList)


This should be limited to the request namespace so that we are only checking for duplicates in the context of one namespace. You can have tag foo in two different namespaces, that is allowed.

should be limited now

ibuildthecloud · 2022-12-16T03:56:29Z

pkg/server/registry/images/strategy.go

-	image := obj.(*apiv1.Image)
-	duplicateTag := make(map[string]bool)
-
-	for _, tag := range image.Tags {


On update we should still validate that the new tag list is unique. Is that check still enforced?

I added it back & changed the order in which the image/tags runs. It will now update all duplicate tags and then either create the new image or update the existing one w/ the tags

ibuildthecloud · 2022-12-16T04:08:41Z

pkg/apis/api.acorn.io/v1/types.go

@@ -171,7 +168,7 @@ type ImageTag struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

-	TagName string `json:"tagName,omitempty"`
+	Image *Image `json:"image,omitempty"`


Suggested change

Image *Image `json:"image,omitempty"`

Tags []string `json:"tags,omitempty"`

The other fields of image, namely Digest don't seem to have any use, lets just have the one field Tags

I changed to Image *Image with image/tags now having to call the create so now the requestImage can be created if it does not exist

func (t *TagStrategy) ImageTag(ctx context.Context, namespace, imageName string, requestImage *apiv1.Image) (*apiv1.Image, error) { image := &apiv1.Image{} imageList := &apiv1.ImageList{} err := t.client.Get(ctx, router.Key(namespace, imageName), image) if apierror.IsNotFound(err) { err = t.client.Create(ctx, requestImage) if err != nil { return image, err }

This is a side effect of the fact i've rewritten things. We don't want the user to be able to create new Image objects, but right now you have to create images from the client. So I guess just leave it as is and I'll refactor it in my PR.

ibuildthecloud · 2022-12-16T04:13:13Z

pkg/cli/images.go

+	Name       string `json:"name,omitempty"`
+	Digest     string `json:"digest,omitempty"`
+	Repository string `json:"repository,omitempty"`
+	Tag        string `json:"tags,omitempty"`


Suggested change

Tag string `json:"tags,omitempty"`

Tag string `json:"tag,omitempty"`

Signed-off-by: Joshua Silverio <joshua@acorn.io>

…rences acorn-io#958 acorn-io#882 (acorn-io#959) refactor: image api to create one img that includes list of tags/references acorn-io#958 Signed-off-by: Joshua Silverio <joshua@acorn.io>

jsilverio22 self-assigned this Dec 2, 2022

jsilverio22 force-pushed the refactor-img branch 2 times, most recently from b5cf37c to 2a6467e Compare December 2, 2022 23:00

jsilverio22 marked this pull request as ready for review December 2, 2022 23:08

jsilverio22 requested review from ibuildthecloud, cjellick and tylerslaton December 2, 2022 23:08

jsilverio22 changed the title ~~refactor: image api to create one img that includes list of tags/references #958~~ refactor: image api to create one img that includes list of tags/references #958 #882 Dec 2, 2022

jsilverio22 force-pushed the refactor-img branch 2 times, most recently from 8b14f68 to fe3f063 Compare December 5, 2022 15:42

tylerslaton suggested changes Dec 5, 2022

View reviewed changes

pkg/server/registry/images/storage.go Outdated Show resolved Hide resolved

integration/client/images/images_test.go Outdated Show resolved Hide resolved

pkg/cli/images.go Outdated Show resolved Hide resolved

pkg/client/image.go Outdated Show resolved Hide resolved

jsilverio22 requested a review from tylerslaton December 5, 2022 17:48

tylerslaton approved these changes Dec 5, 2022

View reviewed changes

pkg/client/image.go Outdated Show resolved Hide resolved

ibuildthecloud reviewed Dec 5, 2022

View reviewed changes

pkg/client/client.go Outdated Show resolved Hide resolved

jsilverio22 force-pushed the refactor-img branch from 2bbfecb to b522e13 Compare December 5, 2022 20:57

cjellick reviewed Dec 5, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

cjellick reviewed Dec 5, 2022

View reviewed changes

pkg/client/image.go Outdated Show resolved Hide resolved

cjellick reviewed Dec 6, 2022

View reviewed changes

pkg/client/image.go Outdated Show resolved Hide resolved

jsilverio22 force-pushed the refactor-img branch from 5f7ef3f to 30fbf02 Compare December 7, 2022 15:21

cjellick reviewed Dec 7, 2022

View reviewed changes

pkg/apis/api.acorn.io/v1/types.go Outdated Show resolved Hide resolved

tylerslaton approved these changes Dec 7, 2022

View reviewed changes

pkg/cli/images_rm.go Outdated Show resolved Hide resolved

pkg/client/image.go Outdated Show resolved Hide resolved

pkg/client/image.go Outdated Show resolved Hide resolved

pkg/cli/images.go Outdated Show resolved Hide resolved

cjellick suggested changes Dec 7, 2022

View reviewed changes

jsilverio22 force-pushed the refactor-img branch from e88ad42 to a823a6b Compare December 10, 2022 06:55