WIP: POC: Leverage of autovar kustomize feature by kubeflow #502

jbrette · 2019-10-11T16:33:25Z

Which issue is resolved by this Pull Request:
Resolves #

DO NOT MERGE. This PR is only here to highlight the potential usage of autovar feature by kubeflow.

$(ConfigMap.parameters.data.xxx) is simplier. No need to change add the varReference nor the var in the kustomization.yaml. The configMapGenerator is still used.
$(KfDef.app.applications[name=xxx].kustomizeConfig.parameters[name=xxx].Value: This avoid using intermediate config map and allows to point a central variable CRD.

Description of your changes:

Checklist:

Unit tests have been rebuilt:
1. cd manifests/tests
2. make generate
3. make test

This change is

…gressgateway (kubeflow#383)" This reverts commit 8196f65.

- update names in kfdef - update tree structure in kfdef - start to use autovar with ConfigMap - start to use autovar with KfDef

k8s-ci-robot · 2019-10-11T16:33:39Z

Hi @jbrette. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-10-11T16:33:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign zhenghuiwang
You can assign the PR to them by writing /assign @zhenghuiwang in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jbrette · 2019-10-11T16:45:39Z

@kkasravi @jlewi @yanniszark Please have look at the proposed code changes. This will give you an idea how the autovar feature we created for our project could be useful for kubeflow

kkasravi · 2019-10-11T20:56:16Z

/ok-to-test

kkasravi · 2019-10-11T20:57:56Z

awesome @jbrette

Does this include changes from Add SMP attributes to KfDef #4181?

k8s-ci-robot · 2019-10-11T21:00:17Z

@jbrette: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
kubeflow-manifests-presubmit	`46f6075`	link	`/test kubeflow-manifests-presubmit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kkasravi

Do we need airshipit in the group of the CRD?
Could we generate the CRD and schema using a different tool?

Is kfdef/kfctl_gcp_iap/kfdef-app.yaml the output of running kustomize build kfdef/kfctl_gcp_iap?

Are the builtin transformers under kfdef/kfctl_gcp_iap applied automatically?
Is KindOrderTransformer equivalent to kustomize build --reorder none?

Since kfctl generates the kustomization.yaml for all top-level manifests what if we were to
define top-level manifests that (taking /manifests/gcp/iap-ingress as an example) look like

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- base
- overlays/managed-cert/cert.yaml
commonLabels:
  kustomize.component: iap-ingress
generators:
- kfdef/kfctl_gcp_iap/kfdef-app.yaml

and the plugin does the generation rather than kfctl?

kkasravi · 2019-10-12T15:44:25Z

gcp/iap-ingress/base/backend-config.yaml

@@ -6,4 +6,4 @@ spec:
  iap:
    enabled: true
    oauthclientCredentials:
-      secretName: $(oauthSecretName)
+      secretName: $(ConfigMap.parameters.data.oauthSecretName)


Other manifest targets also use the name 'parameters' in configMapGenerator.
Should we make each ConfigMap name unique?

kkasravi · 2019-10-12T15:52:55Z

gcp/iap-ingress/base/cloud-endpoint.yaml

-    name: $(ingressName)
-    namespace: $(istioNamespace)
+    name: $(ConfigMap.parameters.data.ingressName)
+    namespace: $(ConfigMap.parameters.data.istioNamespace)


I think we can do

KfDef.app.spec.applications[name=iap-ingress].kustomizeConfig.parameters[name=namespace].value

in config-map.yaml where we are using ConfigMap.parameters.data.namespace

This is fine. I left both options so that the team can understand what is possible. My project is more using the approach of a central index KfDef rather than small ConfigMap.

kkasravi · 2019-10-12T15:59:34Z

gcp/iap-ingress/base/params.env

i think it would be better to move these vars into KfDef.app.applications[name=iap-ingress].kustomizeConfig.parameters and reference them in the ConfigMap

kkasravi · 2019-10-12T16:03:29Z

tests/aws-efs-csi-driver-base_test.go

@@ -138,8 +134,7 @@ subjects:
 roleRef:
  kind: ClusterRole
  name: efs-csi-node-clusterrole
-  apiGroup: rbac.authorization.k8s.io
-`)
+  apiGroup: rbac.authorization.k8s.io`)


I think these diffs are related to go fmt and can probably be ignored

I do believe there are small trailing white spaces in the yaml, or trailing end of file/line character. I think somebody should run a beauty printer on the yaml files and regenerate the go code.

kkasravi · 2019-10-12T16:30:00Z

kfdef/common/kfdef.apps.kubeflow.org_kfdefs.yaml

+kind: CustomResourceDefinition
+metadata:
+  creationTimestamp: null
+  name: kfdefs.kubeflow.airshipit.org


can you provide some background on the mapping between kfdef/kfctl_gcp_iap.yaml and the CRD generation.

kkasravi · 2019-10-12T16:31:14Z

kfdef/kfctl_gcp_iap/kfdef-app.yaml

@@ -1,9 +1,8 @@
 # Please set project and email!
-apiVersion: kfdef.apps.kubeflow.org/v1alpha1
+apiVersion: kubeflow.airshipit.org/v1beta1


was the CRD generated by airshipctl?

First, this PR should not be merged, the airshipit is just a temporary hack.

Indeed I tried to get a PR merged in kubeflow/kubeflow but:

I did not really understood what is using the v1alpha1 and what is using v1beta1.

I ended up having circular dependencies on kustomize, and logrus...all the things which in kubeflow/bootstrap/go.mod

If you check a lot of the projects involving CRDs, they often have the API in a standalone go module. This should be possible long term with the kubeflow/kfctl package. To get the POC going, I copy pasted the KfDef.go here and get it to work in standalone. An example on how to use the newest version of controller-gen is here

What is also intersting is that we could run that script against the CRD known in kfctl. This generates the kubeval.json which also to verify that the syntax of the objects are correct even before running the "kubectl apply". For instance look at here

As for the SMP anotation, it could be really usefull to apply kustomize to the kfdefs.
Now that kustomize is a standalone library, we just have to "register" the types in kfctls liked I tried here.

The examples of merge are here for Argo Workflow, Rollout, KfDef...

jbrette · 2019-10-15T00:57:04Z

@kkasravi @jlewi I was explicitly told today that autovar feature will not be merged in kustomize because there is a $ which looks like too much like template and that would prevent kubectl apply -f

for working. I was forced to withdraw the PR that was left rotten since 4 months. There is a new proposal involving adding even more complicated field structures in the kustomization.yaml. I don't think that is what you need but would you will to adapt kfctl because the complete removal of variable support by kustomize is likely to follow.

yanniszark · 2019-10-15T11:50:53Z

@jbrette could you give a link to the discussion?
I'd be very interested to hear the reasons for rejecting it and what alternative is proposed.

jbrette · 2019-10-15T13:15:44Z

kfctl will soon be in an interesting situation

bases: is deprecated. This will impact quite a kfctl because it uses it to distinguish files from folder.
patchesStrategicMerge will soon be deprecated: deprecate fields: patchesStrategicMerge and patchesJson6902 kubernetes-sigs/kustomize#1517. This will impact kfctl
MakeFakeFs has been renamed and moved around. go.mod in kubeflow and the manifests is what is currently allowing kfctl and the tests to still compile and run.
The pkg folder is about to be drained: Start pluglib, a set of public, plugin specific functions. kubernetes-sigs/kustomize#1633. Note really sure how much was is considered internal components is used by kfctl.
The replacement POC is here: Replacement poc kubernetes-sigs/kustomize#1631, which the structure. Since this is inside each kustomization.yaml (for right now), no really sure how kfctl could leverage the global index/coordinator kfdef.yaml resource concept.
Feature: FieldSpecs, NameBackReferences and Transformers improvements kubernetes-sigs/kustomize#1626 (comment) describes the fact that VAR will be most likely be removed. The main issues seems to be the $( ) form, which is visible before you run kustomize build (not after), but if you step back you can't run "kubectl apply -f ", you are forced to run "kubectl apply -k <dir-containing-a-kustomization.yaml>. In same spirit, something like the commonLabels allows you to leave an invalid deployment.yaml (for instance missing the selector field, which would trigger the kubectl apply -f deployment.yaml), but kustomize build adds it.

So I always tried to contribute to kubeflow because it helped to understand how to use kustomize at scale, but sometimes I wonder how kfctl would be behave it was internally doing something like "helm template xxx | kubectl apply -f" instead of "kustomize build xxx | kubectl apply -f"

jbrette added 3 commits October 11, 2019 15:43

Revert "kfdef: generic: revert kubeflow#376 because it breaks istio-e…

63dc14b

…gressgateway (kubeflow#383)" This reverts commit 8196f65.

Kustomize related improvment: Cleanup the Tiller stuff

81fbb85

KfDef as CRD

46f6075

- update names in kfdef - update tree structure in kfdef - start to use autovar with ConfigMap - start to use autovar with KfDef

k8s-ci-robot added the do-not-merge/work-in-progress label Oct 11, 2019

googlebot added the cla: yes label Oct 11, 2019

k8s-ci-robot added needs-ok-to-test size/XXL labels Oct 11, 2019

k8s-ci-robot requested review from IronPan and yanniszark October 11, 2019 16:33

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 11, 2019

kkasravi suggested changes Oct 12, 2019

View reviewed changes

jbrette closed this Oct 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: POC: Leverage of autovar kustomize feature by kubeflow #502

WIP: POC: Leverage of autovar kustomize feature by kubeflow #502

jbrette commented Oct 11, 2019 •

edited by jlewi

Loading

k8s-ci-robot commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

jbrette commented Oct 11, 2019

kkasravi commented Oct 11, 2019

kkasravi commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

kkasravi left a comment

kkasravi Oct 12, 2019

kkasravi Oct 12, 2019

jbrette Oct 12, 2019

kkasravi Oct 12, 2019

kkasravi Oct 12, 2019

jbrette Oct 12, 2019

kkasravi Oct 12, 2019

kkasravi Oct 12, 2019

jbrette Oct 12, 2019

jbrette commented Oct 15, 2019 •

edited

Loading

yanniszark commented Oct 15, 2019 •

edited

Loading

jbrette commented Oct 15, 2019

WIP: POC: Leverage of autovar kustomize feature by kubeflow #502

WIP: POC: Leverage of autovar kustomize feature by kubeflow #502

Conversation

jbrette commented Oct 11, 2019 • edited by jlewi Loading

k8s-ci-robot commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

jbrette commented Oct 11, 2019

kkasravi commented Oct 11, 2019

kkasravi commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

kkasravi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrette commented Oct 15, 2019 • edited Loading

yanniszark commented Oct 15, 2019 • edited Loading

jbrette commented Oct 15, 2019

jbrette commented Oct 11, 2019 •

edited by jlewi

Loading

jbrette commented Oct 15, 2019 •

edited

Loading

yanniszark commented Oct 15, 2019 •

edited

Loading