Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Bump FlytePlugins version #469

Merged
merged 24 commits into from
Aug 15, 2022
Merged

Bump FlytePlugins version #469

merged 24 commits into from
Aug 15, 2022

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Aug 9, 2022

Signed-off-by: Kevin Su [email protected]

TL;DR

  • Bump flyteplugins version to support ray tasks
  • Updated args in manager.Options{} since ClientBuilder is removed from the latest version of controller-runtime
  • Replace ConfigMapsResourceLock with ConfigMapsLeasesResourceLock since ConfigMapsResourceLock is removed

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#2641

Follow-up issue

NA

pkg/controller/executors/kube.go Outdated Show resolved Hide resolved
cmd/controller/cmd/root.go Show resolved Hide resolved
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #469 (18d5dd5) into master (0fc2b9f) will not change coverage.
The diff coverage is 50.00%.

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@hamersaw hamersaw self-requested a review August 12, 2022 18:13
hamersaw
hamersaw previously approved these changes Aug 12, 2022
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@wild-endeavor
Copy link
Contributor

@hamersaw do you know why this test runs for so long? is it supposed to take more than six hours?

@hamersaw
Copy link
Contributor

@hamersaw do you know why this test runs for so long? is it supposed to take more than six hours?

@wild-endeavor looks like the codeQL check - looking into it.

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
This reverts commit 0ea7a9f.
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@hamersaw hamersaw self-requested a review August 15, 2022 16:52
@pingsutw pingsutw merged commit 4938905 into master Aug 15, 2022
@pingsutw pingsutw deleted the ray-job branch August 15, 2022 17:00
@@ -41,7 +41,7 @@ func NewResourceLock(corev1 v1.CoreV1Interface, coordinationV1 v12.CoordinationV
}

// Leader id, needs to be unique
return resourcelock.New(resourcelock.ConfigMapsResourceLock,
return resourcelock.New(resourcelock.ConfigMapsLeasesResourceLock,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure, but this change seems to require additional permissions:

 apiGroups:
  - coordination.k8s.io
  resources:
  - leases
  verbs: ...

If so, this is a breaking change unfortunately.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, this caused a failed deployment for us because we did not have this RBAC permission in place when upgrading to 1.2.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

@honnix so sorry about not exposing this in the release notes, thanks for tracking it down - it will be added. Are these additional RBAC permissions an issue?

It looks like the ConfigMapLeasesResourceLock has been the default for a very long time and we should have migrated over sooner. Now support for ConfigMapResourceLock has been removed.

Copy link
Member

Choose a reason for hiding this comment

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

@hamersaw No worries. Just wanted to give you folks a heads-up. It was fairly easy to solve on our side and it is all good now.

eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
* Bump FlytePlugins version

Signed-off-by: Kevin Su <[email protected]>

* use ZZZ_DeprecatedClusterName instead

Signed-off-by: Kevin Su <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

* go generate

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* update workflow name

Signed-off-by: Kevin Su <[email protected]>

* add comment

Signed-off-by: Kevin Su <[email protected]>

* Address comment

Signed-off-by: Kevin Su <[email protected]>

* fix test

Signed-off-by: Kevin Su <[email protected]>

* remove cluster name

Signed-off-by: Kevin Su <[email protected]>

* remove cluster name

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* set timeout

Signed-off-by: Kevin Su <[email protected]>

* update viper

Signed-off-by: Kevin Su <[email protected]>

* Revert "update viper"

This reverts commit 0ea7a9f.

* ping viper 0.9.0

Signed-off-by: Kevin Su <[email protected]>

* updated stdlib

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants