Clean up manifests by tenzen-y · Pull Request #510 · kubeflow/mpi-operator

tenzen-y · 2023-01-27T11:52:48Z

Signed-off-by: Yuki Iwai yuki.iwai.tz@gmail.com

I cleaned up manifests in the following:

Replace the image tag with 'master' to avoid this confusing.
Update the make crd target so that we can add the latest CRD to the manifests directory.
Regenerate deploy/v2beta1/mpi-operator.yaml.
Update the Readme.md.
Add CI to verify generated codes.

Our operator images at dockerhub:

Fixes: #508

tenzen-y · 2023-01-27T11:53:26Z

/assign @terrytangyuan

tenzen-y · 2023-01-27T17:17:26Z

@terrytangyuan Or we push the latest image with latest tag by changing Ci?

mpi-operator/.github/workflows/mpi-operator-docker-image-publish.yml

Lines 29 to 33 in 05ac6ad

    
                     tags: | 
        
                       type=ref,event=branch 
        
                       type=ref,event=pr 
        
                       type=semver,pattern={{version}} 
        
                       type=semver,pattern={{major}}.{{minor}}

cc: @alculquicondor

Updated

I modified the CI to push the latest image with the latest image tag.

alculquicondor · 2023-01-27T18:15:15Z

.github/workflows/mpi-operator-docker-image-publish.yml

            type=ref,event=pr
            type=semver,pattern={{version}}
            type=semver,pattern={{major}}.{{minor}}
+            type=raw,latest


but wouldn't this affect every tag?

Does that mean the existing latest tag image is overwritten?

IIUC, by doing this the latest tag would always be overriden with every push. I think we should do this manually for new releases only.

Makes sense. What do you think about replacing the image tag with master in the following to avoid this confusing?

mpi-operator/manifests/overlays/standalone/kustomization.yaml

Line 15 in 05ac6ad

newTag: latest

that sounds better to me.

The manifests that are part of a release should have the specific release tag.

Sounds good. I would update this PR.

tenzen-y · 2023-01-27T20:42:59Z

@alculquicondor Updated.

alculquicondor · 2023-01-30T15:17:02Z

Can we update https://github.com/kubeflow/mpi-operator#installation to indicate that this will install the latest development version?

:latest is still the latest release, 0.3.0. I wonder if we should have stored those manifests somewhere for users reference. Can we still add them as a release asset? @terrytangyuan

tenzen-y · 2023-01-30T16:09:20Z

Can we update https://github.com/kubeflow/mpi-operator#installation to indicate that this will install the latest development version?

Makes sense. It is helpful.

tenzen-y · 2023-01-30T16:24:14Z

@alculquicondor I updated the docs.

alculquicondor · 2023-01-30T16:42:27Z

README.md

-git clone https://github.com/kubeflow/mpi-operator
-cd mpi-operator
-kubectl apply -f deploy/v2beta1/mpi-operator.yaml
+kubectl apply -f https://raw.githubusercontent.com/kubeflow/mpi-operator/master/deploy/v2beta1/mpi-operator.yaml


this file still says latest actually

mpi-operator/deploy/v2beta1/mpi-operator.yaml

Line 483 in 382da78

image: mpioperator/mpi-operator:latest

regenerate the file

I plan to generate that manifest in #509.
Do you prefer to regenerate the manifests in this PR?

Sure. I close #508 and regenerate manifests in this PR.

tenzen-y · 2023-01-30T17:46:16Z

.github/workflows/main.yml

+      - name: generate codes
+        run: make verify-generate


As discussed in #509 (review), we verify generated codes.

alculquicondor · 2023-01-30T17:55:29Z

Makefile

 	$(GOLANGCI_LINT) run --new-from-rev=origin/master --go 1.19

+# Generate deploy/v2beta1/mpi-operator.yaml
+all-in-one: kustomize crd


all-in-one sounds very cryptic. What about manifest

Sounds good.

alculquicondor · 2023-01-30T17:59:46Z

deploy/v2beta1/mpi-operator.yaml

    plural: mpijobs
-    shortNames:


should we add the shortname via kubebuilder markers? https://book.kubebuilder.io/reference/markers/crd.html
I say no, given that these names are too short to be meaningful

I agree with you. I also think we don't need these shortNames.
Let me know what other members think.

cc: @terrytangyuan

alculquicondor · 2023-01-30T18:06:45Z

deploy/v2beta1/mpi-operator.yaml

-                    type: object
-                required:
-                - Launcher
+                additionalProperties:


We are losing the knowledge about Launcher and Worker here.
But this is a problem with the API structs themselves.

We shouldn't be using a map https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps

Maybe we can fix it in a v2(beta2) API, but it would be nice to fix other training job objects, like so:

type MPIReplicaSpecs struct { Launcher common.ReplicaSpec `json:"launcher"` Worker common.ReplicaSpec `json:"worker"` }

Looks good.
IIRC, we plan to remove dependencies with kubeflow/common in the future altogether.
So we will be able to fix this after that.

Either way, I create an issue to keep tracking this.

tenzen-y · 2023-02-04T19:39:09Z

@alculquicondor This PR is ready for review. PTAL.

alculquicondor

/lgtm

alculquicondor · 2023-02-06T17:59:41Z

pkg/controller/mpi_job_controller_status.go

 // initializeMPIJobStatuses initializes the ReplicaStatuses for MPIJob.
 func initializeMPIJobStatuses(mpiJob *kubeflow.MPIJob, mtype kubeflow.MPIReplicaType) {
-	replicaType := kubeflow.MPIReplicaType(mtype)
+	replicaType := mtype


just remove this line?

Makes sense.

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y · 2023-02-06T18:07:04Z

Addressed comments and squashed commits into one.

alculquicondor · 2023-02-06T18:08:54Z

/lgtm

alculquicondor · 2023-02-06T18:16:24Z

/assign @terrytangyuan

tenzen-y · 2023-02-08T17:03:10Z

@terrytangyuan Can you add a approve label to this PR? We need this improvement for the v0.4.0 release.

alculquicondor · 2023-02-08T17:51:30Z

Let me try my new powers 😂

/approve

google-oss-prow · 2023-02-08T17:51:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [alculquicondor]

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

tenzen-y · 2023-02-08T17:59:38Z

Let me try my new powers 😂

/approve

Great power 🚀

google-oss-prow bot requested review from carmark and gaocegege January 27, 2023 11:52

google-oss-prow bot added the size/XS label Jan 27, 2023

google-oss-prow bot assigned terrytangyuan Jan 27, 2023

tenzen-y mentioned this pull request Jan 27, 2023

[WIP] Generate the all-in-one manifest #509

Closed

tenzen-y force-pushed the change-image-tag branch from d38ff6b to df5496b Compare January 27, 2023 17:45

tenzen-y changed the title ~~Change the image tag to master~~ Push the latest image with image tag Jan 27, 2023

alculquicondor reviewed Jan 27, 2023

View reviewed changes

tenzen-y force-pushed the change-image-tag branch from df5496b to 77989c6 Compare January 27, 2023 20:40

tenzen-y changed the title ~~Push the latest image with image tag~~ Use 'master' tag for the operator image instead of 'latest' Jan 27, 2023

tenzen-y force-pushed the change-image-tag branch from 77989c6 to 1f4c681 Compare January 30, 2023 16:22

google-oss-prow bot added size/S and removed size/XS labels Jan 30, 2023

alculquicondor reviewed Jan 30, 2023

View reviewed changes

tenzen-y changed the title ~~Use 'master' tag for the operator image instead of 'latest'~~ [WIP] Use 'master' tag for the operator image instead of 'latest' Jan 30, 2023

google-oss-prow bot added the do-not-merge/work-in-progress label Jan 30, 2023

tenzen-y force-pushed the change-image-tag branch from 1f4c681 to f30ee74 Compare January 30, 2023 17:41

google-oss-prow bot added size/XXL and removed size/S labels Jan 30, 2023

tenzen-y force-pushed the change-image-tag branch from f30ee74 to 8df35aa Compare January 30, 2023 17:44

tenzen-y commented Jan 30, 2023

View reviewed changes

tenzen-y changed the title ~~[WIP] Use 'master' tag for the operator image instead of 'latest'~~ Cleanup manifests Jan 30, 2023

google-oss-prow bot removed the do-not-merge/work-in-progress label Jan 30, 2023

alculquicondor reviewed Jan 30, 2023

View reviewed changes

alculquicondor mentioned this pull request Feb 2, 2023

Support suspend semantics for MPIJob #511

Merged

tenzen-y force-pushed the change-image-tag branch 4 times, most recently from f9589f8 to a1d0bf9 Compare February 4, 2023 18:36

tenzen-y changed the title ~~[WIP] Cleanup manifests~~ Cleanup manifests Feb 4, 2023

google-oss-prow bot removed the do-not-merge/work-in-progress label Feb 4, 2023

alculquicondor reviewed Feb 6, 2023

View reviewed changes

google-oss-prow bot assigned alculquicondor Feb 6, 2023

google-oss-prow bot added the lgtm label Feb 6, 2023

Clean up manifests

37967bc

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y force-pushed the change-image-tag branch from a1d0bf9 to 37967bc Compare February 6, 2023 18:06

google-oss-prow bot removed the lgtm label Feb 6, 2023

tenzen-y changed the title ~~Cleanup manifests~~ Clean up manifests Feb 6, 2023

tenzen-y requested review from alculquicondor and removed request for carmark and gaocegege February 6, 2023 18:07

google-oss-prow bot added the lgtm label Feb 6, 2023

tenzen-y mentioned this pull request Feb 7, 2023

Set the knowledge about Launcher and Worker to CRD #519

Open

google-oss-prow bot added the approved label Feb 8, 2023

google-oss-prow bot merged commit efbba01 into kubeflow:master Feb 8, 2023

tenzen-y deleted the change-image-tag branch February 8, 2023 17:58

Comments

Conversation

tenzen-y commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tenzen-y commented Jan 27, 2023

Uh oh!

tenzen-y commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y commented Jan 27, 2023

Uh oh!

alculquicondor commented Jan 30, 2023

Uh oh!

tenzen-y commented Jan 30, 2023

Uh oh!

tenzen-y commented Jan 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y commented Feb 4, 2023

Uh oh!

alculquicondor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y commented Feb 6, 2023

Uh oh!

alculquicondor commented Feb 6, 2023

Uh oh!

alculquicondor commented Feb 6, 2023

Uh oh!

tenzen-y commented Feb 8, 2023

Uh oh!

alculquicondor commented Feb 8, 2023

Uh oh!

google-oss-prow bot commented Feb 8, 2023

Uh oh!

tenzen-y commented Feb 8, 2023

Uh oh!

Reviewers

tenzen-y commented Jan 27, 2023 •

edited

Loading

tenzen-y commented Jan 27, 2023 •

edited

Loading

tenzen-y Jan 30, 2023 •

edited

Loading