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

[Bug Fix] Add default tag (.Chart.AppVersion) to graphscope-store #2205

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

dashanji
Copy link
Collaborator

@dashanji dashanji commented Nov 4, 2022

While installing the graphscope helm chart, as the tag is nil, the image is rendered as registry.cn-hongkong.aliyuncs.com/graphscope/graphscope: so we can't install it as expected.

@sighingnow
Copy link
Collaborator

The image tag was expected to be Chart.AppVersion: https://github.com/alibaba/GraphScope/blob/main/charts/graphscope/templates/coordinator.yaml#L22

Don't use latest as tag please as it hard to ensure compatibility with the latest image version and the old chart versions.

@sighingnow
Copy link
Collaborator

If AppVersion is not rendered correctly, I suggest you figure out why it is not rendered, rather than changing the default value to "latest".

@dashanji
Copy link
Collaborator Author

dashanji commented Nov 7, 2022

Make sense, the latest commit is to add default tag(Chart.AppVersion) to graphscope-store's helm chart.

It fix the following error:

$ helm install graphscope-store "./graphscope-store"
Error: INSTALLATION FAILED: YAML parse error on graphscope-store/templates/coordinator/statefulset.yaml: error converting YAML to JSON: yaml: line 39: mapping values are not allowed in this context

Signed-off-by: dashanji <caoye.cao@alibaba-inc.com>
Signed-off-by: dashanji <caoye.cao@alibaba-inc.com>
@@ -86,6 +86,13 @@ Return the proper image name
{{- $registryName = .global.imageRegistry -}}
{{- end -}}
{{- end -}}
{{- if not $tag }}
{{- if .imageRoot.DefaultTag }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the new kv you added isn't used ("DefaultTag" .Chart.AppVersion)?
The .imageRoot.DefaultTag points to the image: DefaultTag in values.yaml?
So I guess if you install the charts without tag specified, you will have latest tag instead of 0.17.0 or 0.18.0 (the AppVersion)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should change the .imageRoot.DefaultTag to .DefaultTag?

@@ -70,12 +70,12 @@ app.kubernetes.io/instance: {{ .Release.Name }}
Return the proper graphscope-store image name
*/}}
{{- define "graphscope-store.image" -}}
{{ include "graphscope-store.images.image" (dict "imageRoot" .Values.image "global" .Values.global) }}
{{ include "graphscope-store.images.image" (dict "imageRoot" .Values.image "global" .Values.global "DefaultTag" .Chart.AppVersion ) }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siyuan0322 The new kv is contained in the dict, and I think the imageRoot.DefaultTag points to the .Chart.AppVersion of the Chart.yaml. Is there something I missed?

In fact, we can get the following result.

$ helm install graphscope-store "./graphscope-store"
$ kubectl get po coordinator-graphscope-6d8fccbb9c-cl6vm -oyaml | grep image
    - --k8s_gs_image
    - --k8s_etcd_image
    - --k8s_image_pull_policy
    - --k8s_image_pull_secrets
    image: registry.cn-hongkong.aliyuncs.com/graphscope/graphscope:0.18.0
    imagePullPolicy: Always
  - image: registry.cn-hongkong.aliyuncs.com/graphscope/jupyter:0.18.0
    imagePullPolicy: Always

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, we have two charts here, graphscope and graphscope-store, I thought that make you confused a bit 😢 Here the pod coordinator-graphscope-6d8fccbb9c-cl6vm is from your previous install graphscope charts, not graphscope-store

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catching, I have tried again and find the tag is exactly latest.

$ kubectl get po graphscope-store-coordinator-0 -n gh-store -oyaml | grep image
            f:image: {}
            f:imagePullPolicy: {}
    image: registry.cn-hongkong.aliyuncs.com/graphscope/graphscope-store:latest
    imagePullPolicy: IfNotPresent
    image: registry.cn-hongkong.aliyuncs.com/graphscope/graphscope-store:latest
    imageID: registry.cn-hongkong.aliyuncs.com/graphscope/graphscope-store@sha256:b00a5a15a48a5bca86d02f7c16a07335377bc3e617b3d67c22331b5a5ba9008a

I'll fix it ASAP.

@codecov-commenter
Copy link

Codecov Report

Merging #2205 (8ee47bf) into main (4279b8f) will increase coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2205      +/-   ##
==========================================
+ Coverage   40.01%   40.03%   +0.02%     
==========================================
  Files          87       87              
  Lines        9752     9752              
==========================================
+ Hits         3902     3904       +2     
+ Misses       5850     5848       -2     
Impacted Files Coverage Δ
python/graphscope/deploy/kubernetes/utils.py 70.47% <0.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4279b8f...8ee47bf. Read the comment docs.

@sighingnow sighingnow merged commit 094e81d into alibaba:main Nov 8, 2022
@sighingnow sighingnow changed the title [Bug Fix] Add latest tag to all image in graphscope's helm chart [Bug Fix] Add default tag (.Chart.AppVersion) to graphscope-store Nov 8, 2022
@dashanji dashanji deleted the add-image-tag branch November 8, 2022 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants