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

chore: Revert "feat: Machine Migration (#176)" #241

Conversation

jonathan-innis
Copy link
Member

@jonathan-innis jonathan-innis commented Mar 15, 2023

Fixes #

Description

Revert relevant changes in #176. Keeps some of the changes in the PR, but removes any changes that change the architecture from creating Nodes to modeling inflight nodes as machines.

How was this change tested?

  • make presubmit
  • Manual Consolidation Testing
  • Manual Inflight Check Testing
  • FOCUS=Integration make e2etests succeeded
  • Validating metrics through manual testing with Prometheus

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jonathan-innis jonathan-innis requested a review from a team as a code owner March 15, 2023 00:36
@jonathan-innis jonathan-innis force-pushed the revert-machine-migration-changes branch 3 times, most recently from 354c31c to b2ac9a9 Compare March 15, 2023 00:43
@coveralls
Copy link

coveralls commented Mar 15, 2023

Pull Request Test Coverage Report for Build 4429562127

  • 193 of 244 (79.1%) changed or added relevant lines in 22 files are covered.
  • 83 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.1%) to 81.032%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/metrics/metrics.go 0 1 0.0%
pkg/controllers/deprovisioning/expiration.go 1 3 33.33%
pkg/controllers/inflightchecks/controller.go 9 11 81.82%
pkg/controllers/node/finalizer.go 11 13 84.62%
pkg/controllers/deprovisioning/drift.go 1 4 25.0%
pkg/controllers/deprovisioning/consolidation.go 7 11 63.64%
pkg/controllers/deprovisioning/controller.go 14 18 77.78%
pkg/controllers/deprovisioning/events/events.go 9 13 69.23%
pkg/controllers/node/initialization.go 48 53 90.57%
pkg/controllers/inflightchecks/failedinit.go 16 23 69.57%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/inflightchecks/nodeshape.go 1 76.92%
pkg/controllers/provisioning/provisioner.go 1 77.81%
pkg/controllers/deprovisioning/types.go 2 81.9%
pkg/controllers/state/statenode.go 2 94.57%
pkg/controllers/termination/controller.go 3 49.06%
pkg/controllers/state/informer/machine.go 5 59.26%
pkg/controllers/provisioning/scheduling/preferences.go 7 86.67%
pkg/controllers/deprovisioning/controller.go 9 77.01%
pkg/controllers/state/cluster.go 23 84.47%
pkg/test/expectations/expectations.go 30 87.17%
Totals Coverage Status
Change from base Build 4408302250: 0.1%
Covered Lines: 6579
Relevant Lines: 8119

💛 - Coveralls

@jonathan-innis jonathan-innis force-pushed the revert-machine-migration-changes branch 4 times, most recently from b300a7f to ed0bb76 Compare March 15, 2023 01:26
@jonathan-innis jonathan-innis enabled auto-merge (squash) March 15, 2023 01:39
@jonathan-innis jonathan-innis force-pushed the revert-machine-migration-changes branch 3 times, most recently from 00da3da to 3ac320f Compare March 15, 2023 16:59
bwagner5
bwagner5 previously approved these changes Mar 15, 2023
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

LGTM

bwagner5
bwagner5 previously approved these changes Mar 15, 2023
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathan-innis jonathan-innis force-pushed the revert-machine-migration-changes branch 3 times, most recently from 60f758c to 40c3cf6 Compare March 15, 2023 18:11
@jonathan-innis jonathan-innis force-pushed the revert-machine-migration-changes branch from 40c3cf6 to 25df41e Compare March 15, 2023 18:12
@jonathan-innis jonathan-innis enabled auto-merge (squash) March 15, 2023 18:24
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathan-innis jonathan-innis merged commit 9973eac into kubernetes-sigs:main Mar 15, 2023
go.mod Show resolved Hide resolved
Expect(nodes[0].Name).ToNot(Equal(node.Name))
})
It("should uncordon nodes when expiration replacement partially fails", func() {
It("should uncordon nodes when expiration replacement fails", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you want to change this test? Was this how it was before?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is effectively the same test, since we are validating the ability of the test to uncordon after we decide to launch. Whether it's 3 or 1 I don't think makes much of a difference here

@jonathan-innis jonathan-innis deleted the revert-machine-migration-changes branch March 15, 2023 18:27
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Mar 16, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Mar 16, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 4, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 5, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 8, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 10, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 11, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 12, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 13, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 14, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 18, 2023
jonathan-innis added a commit to jonathan-innis/karpenter that referenced this pull request Apr 20, 2023
jonathan-innis added a commit that referenced this pull request Apr 21, 2023
* Revert "Revert machine migration changes (#176) (#241)"

This reverts commit 9973eac.

* Change owner reference to blockOwnerDeletion

* Remove string logging for machine launch

* Removing FailedInit since machine statusCondition captures it

* Updated eventing on machines
njtran pushed a commit to njtran/karpenter that referenced this pull request May 3, 2023
* Revert "Revert machine migration changes (kubernetes-sigs#176) (kubernetes-sigs#241)"

This reverts commit 9973eac.

* Change owner reference to blockOwnerDeletion

* Remove string logging for machine launch

* Removing FailedInit since machine statusCondition captures it

* Updated eventing on machines
njtran added a commit that referenced this pull request May 4, 2023
* chore: cleanup deprovisioning types

* add inc

* fix: add more visibility for when we can't consolidate (#292)

* feat: Machine Migration (#273)

* Revert "Revert machine migration changes (#176) (#241)"

This reverts commit 9973eac.

* Change owner reference to blockOwnerDeletion

* Remove string logging for machine launch

* Removing FailedInit since machine statusCondition captures it

* Updated eventing on machines

* action const

* testfix

* testfix2

* comments

* rename

* last

* enum

* fixcompile

* export

* fixredundancy

* fixlogic

* comment

* renameVar

---------

Co-authored-by: Todd Neal <tnealt@amazon.com>
Co-authored-by: Jonathan Innis <jonathan.innis.ji@gmail.com>
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