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

[Manifest] Apply kustomize best practices to standalone manifest #3978

Merged
merged 6 commits into from
Jun 16, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ metadata:
data:
config: |
{
namespace: $(NAMESPACE),
namespace: $(kfp-namespace),
executorImage: gcr.io/ml-pipeline/argoexec:v2.7.5-license-compliance,
artifactRepository:
{
s3: {
bucket: $(BUCKET_NAME),
keyPrefix: artifacts,
endpoint: minio-service.$(NAMESPACE):9000,
endpoint: minio-service.$(kfp-namespace):9000,
insecure: true,
accessKeySecret: {
name: mlpipeline-minio-artifact,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ roleRef:
subjects:
- kind: ServiceAccount
name: kubeflow-pipelines-cache-deployer-sa
namespace: $(NAMESPACE)
# namespace will be added by kustomize automatically according to the namespace field in kustomization.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
apiVersion: v1
kind: ServiceAccount
metadata:
name: kubeflow-pipelines-cache-deployer-sa
name: kubeflow-pipelines-cache-deployer-sa
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@ kind: Kustomization
resources:
- cache-deployer-clusterrole.yaml
- cache-deployer-clusterrolebinding.yaml
# HACK: although a service account(SA) is not a cluster-scoped resource.
# Presence of a SA referred by a clusterrolebinding allows kustomize to auto-add
# namespace for the clusterrolebinding's SA ref.
- cache-deployer-sa.yaml

2 changes: 0 additions & 2 deletions manifests/kustomize/base/cache-deployer/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,4 @@ kind: Kustomization
resources:
- cache-deployer-role.yaml
- cache-deployer-rolebinding.yaml
- cache-deployer-sa.yaml
- cache-deployer-deployment.yaml

2 changes: 1 addition & 1 deletion manifests/kustomize/base/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ secretGenerator:
- name: mysql-secret
env: params-db-secret.env
vars:
- name: NAMESPACE
- name: kfp-namespace
objref:
kind: Deployment
apiVersion: apps/v1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ spec:
- name: OBJECTSTORECONFIG_SECURE
value: "false"
- name: OBJECTSTORECONFIG_BUCKETNAME
value: $(BUCKET_NAME)
valueFrom:
configMapKeyRef:
name: pipeline-install-config
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw in real deployment, the configmap name is pipeline-install-config-xxx. It has a strange post-fix, does it matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No , it won't cause any problems. kustomize adds the suffix automatically to both configmap name and references to it. So that it can support configmap rolling update (updating configmap requires all dependent pods to restart to pick up new changes -- therefore, by adding a suffix, when configmap content changes, all deployments referring to it will use a new configmap name, so they will restart those pods with new configmap.)

configMapKeyRef from a configmap will be preserved in deployment yaml in Kubernetes, that's more kubernetes native way of adding config to a deployment.
This is suggested in https://github.com/kubeflow/manifests/blob/master/docs/KustomizeBestPractices.md#command-line-substitution.

key: bucketName
- name: DBCONFIG_USER
valueFrom:
secretKeyRef:
Expand Down
35 changes: 17 additions & 18 deletions manifests/kustomize/cluster-scoped-resources/kustomization.yaml
Original file line number Diff line number Diff line change
@@ -1,28 +1,27 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

namespace: kubeflow

resources:
- namespace.yaml
bases:
- ../base/application/cluster-scoped
- ../base/argo/cluster-scoped
- ../base/pipeline/cluster-scoped
- ../base/cache-deployer/cluster-scoped

resources:
- namespace.yaml

# Used by Kustomize
configMapGenerator:
- name: pipeline-cluster-scoped-install-config
env: params.env

vars:
- name: NAMESPACE
objref:
kind: ConfigMap
name: pipeline-cluster-scoped-install-config
apiVersion: v1
fieldref:
fieldpath: data.namespace

# NOTE: var name must be unique globally to allow composition of multiple kustomize
# packages. Therefore, we added prefix `kfp-cluster-scoped-` to distinguish it from
# others.
- name: kfp-cluster-scoped-namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering whether we just use "kfp-" prefix as it can already solve the concern of global conflict with other components. "kfp-cluster-sopced-namespace" would let me think on whether there is another namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, any other suggestions?

I wasn't using kfp- because I want to be able to compose cluster-scoped package together with kfp namespace scoped package.
Configuration for a var can only appear once between both packages.

objref:
# cache deployer sa's metadata.namespace will be first transformed by namespace field in kustomization.yaml
# so that we only need to change kustomization.yaml's namespace field for namespace customization.
kind: ServiceAccount
name: kubeflow-pipelines-cache-deployer-sa
apiVersion: v1
fieldref:
fieldpath: metadata.namespace
configurations:
- params.yaml
- params.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
apiVersion: v1
kind: Namespace
metadata:
name: $(NAMESPACE)
name: '$(kfp-cluster-scoped-namespace)'
1 change: 0 additions & 1 deletion manifests/kustomize/cluster-scoped-resources/params.env

This file was deleted.

2 changes: 0 additions & 2 deletions manifests/kustomize/cluster-scoped-resources/params.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,3 @@
varReference:
- path: metadata/name
kind: Namespace
- path: subjects/namespace
kind: ClusterRoleBinding
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

# !!! If you want to customize the namespace,
# please also update sample/kustomization.yaml's namespace field to the same value
namespace: kubeflow

bases:
# Or github.com/kubeflow/pipelines/manifests/kustomize/cluster-scoped-resources?ref=1.0.0
- ../../cluster-scoped-resources

# Change the value in params.env to yours.
configMapGenerator:
- name: pipeline-cluster-scoped-install-config
env: params.env
behavior: merge

This file was deleted.

2 changes: 1 addition & 1 deletion manifests/kustomize/sample/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ secretGenerator:
behavior: merge

# !!! If you want to customize the namespace,
# please also update sample/params.env and sample/cluster-scoped-resources/params.env
# please also update sample/cluster-scoped-resources/kustomization.yaml's namespace field to the same value
namespace: kubeflow

#### Customization ###
Expand Down