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.

Sign up for GitHub

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

Jump to bottom

[IMPROVEMENT] Deprecate the setting allow-node-drain-with-last-healthy-replica and replace it by node-drain-policy setting #5585

Closed
PhanLe1010 opened this issue Mar 17, 2023 · 9 comments
Closed

[IMPROVEMENT] Deprecate the setting allow-node-drain-with-last-healthy-replica and replace it by node-drain-policy setting #5585

PhanLe1010 opened this issue Mar 17, 2023 · 9 comments
Assignees
@PhanLe1010
Labels
backport/1.3.3 backport/1.4.2 kind/improvement Request for improvement of existing function priority/0 Must be implement or fixed in this release (managed by PO) require/doc Require updating the longhorn.io documentation wontfix
Milestone
v1.5.0

Comments

@PhanLe1010
Copy link
Contributor

PhanLe1010 commented Mar 17, 2023 •
edited
Loading

Is your improvement request related to a feature? Please describe (👍 if you like this request)

We introduced the new setting node-drain-policy in longhorn/longhorn-manager#1775. The new setting covers all use cases of the old setting allow-node-drain-with-last-healthy-replica and have extra options. Therefore we should deprecate the setting allow-node-drain-with-last-healthy-replica and replace it by node-drain-policy setting.

Work needs to be done:

  • Document
  • Longhorn manager logic
  • Possibly upgrade/migration if needed
@PhanLe1010 PhanLe1010 added require/doc Require updating the longhorn.io documentation kind/improvement Request for improvement of existing function labels Mar 17, 2023
@PhanLe1010 PhanLe1010 mentioned this issue Mar 17, 2023
Merged
@innobead innobead added this to the v1.5.0 milestone Mar 17, 2023
@PhanLe1010 PhanLe1010 mentioned this issue Mar 20, 2023
Merged
@innobead innobead added the priority/0 Must be implement or fixed in this release (managed by PO) label Apr 13, 2023
@PhanLe1010 PhanLe1010 modified the milestones: v1.5.0, v1.3.3, v1.4.2 Apr 17, 2023
@longhorn-io-github-bot
Copy link

longhorn-io-github-bot commented Apr 17, 2023 •
edited by PhanLe1010
Loading

Pre Ready-For-Testing Checklist

  • Where is the reproduce steps/test steps documented?
    The reproduce steps/test steps are :

    • Make sure that the website document ( deploy/important-notes/ and references/settings/) are correctly updated that the setting Allow Node Drain with the Last Healthy Replica is deprecated
    • Make sure that Longhorn UI setting page indicates that the setting Allow Node Drain with the Last Healthy Replica is deprecated

  • Is there a workaround for the issue? If so, where is it documented?
    The workaround is at:

  • Does the PR include the explanation for the fix or the feature?

  • Does the PR include deployment change (YAML/Chart)? If so, where are the PRs for both YAML file and Chart?
    The PR for the YAML change is at:
    The PR for the chart change is at:

  • Have the backend code been merged (Manager, Engine, Instance Manager, BackupStore etc) (including backport-needed/*)?
    The PR is at
    [v1.4.2] Mark the setting allow-node-drain-with-last-healthy-replica as deprecated longhorn-manager#1856

  • Which areas/issues this PR might have potential impacts on?
    Area: update, node drain
    Issues

  • If labeled: require/LEP Has the Longhorn Enhancement Proposal PR submitted?
    The LEP PR is at

  • If labeled: area/ui Has the UI issue filed or ready to be merged (including backport-needed/*)?
    The UI issue/PR is at

  • If labeled: require/doc Has the necessary document PR submitted or merged (including backport-needed/*)?
    The documentation issue/PR is at Update Node Maintenance and Kubernetes Upgrade Guide website#677

  • If labeled: require/automation-e2e Has the end-to-end test plan been merged? Have QAs agreed on the automation test case? If only test case skeleton w/o implementation, have you created an implementation issue (including backport-needed/*)
    The automation skeleton PR is at
    The automation test case PR is at
    The issue of automation test case implementation is at (please create by the template)

  • If labeled: require/automation-engine Has the engine integration test been merged (including backport-needed/*)?
    The engine automation PR is at

  • If labeled: require/manual-test-plan Has the manual test plan been documented?
    The updated manual test plan is at

  • If the fix introduces the code for backward compatibility Has a separate issue been filed with the label release/obsolete-compatibility?
    The compatibility issue is filed at

@github-actions github-actions bot mentioned this issue Apr 17, 2023
Closed
This was referenced Apr 17, 2023
Merged
Merged
@innobead innobead modified the milestones: v1.4.2, v1.5.0 Apr 27, 2023
@github-actions github-actions bot mentioned this issue Apr 27, 2023
Closed
@yangchiu
Copy link
Member

For Make sure that Longhorn UI setting page indicates that the setting Allow Node Drain with the Last Healthy Replica is deprecated, I'm unable to find any description on Longhorn UI setting page indicates it's deprecated on master-head. Do we have a ticket about this UI change? @innobead

@innobead
Copy link
Member

innobead commented Apr 28, 2023 •
edited
Loading

@PhanLe1010 I believe we need to change the setting to SettingTypeDeprecated. Please help with this change in 1.4.x and 1.3.x.
/types/setting.go#L693

		Type:     SettingTypeBool,
		Required: true,

Also, remove this setting from 1.5.0.

@PhanLe1010
Copy link
Contributor Author

Changing setting type to SettingTypeDeprecated will removed the setting from UI and user will not be able to set it.

Should we do this in 1.3 and 1.4 @innobead ?

@innobead
Copy link
Member

Oh should not. I think the UI should be refined, the deprecated item should not be removed but highlighted instead.

Let's just need to remove this setting from 1.5.0.

I will create an issue for UI improvement for deprecated setting.

@PhanLe1010
Copy link
Contributor Author

the deprecated item should not be removed but highlighted instead

I agree

@yangchiu
Copy link
Member

  • Make sure that Longhorn UI setting page indicates that the setting Allow Node Drain with the Last Healthy Replica is deprecated

About this item, I'm not sure what we're expecting to? Currently this setting is still shown up on UI and without any description of the deprecation. @innobead @PhanLe1010

@innobead
Copy link
Member

In 1.5.0, this old setting should be removed, since it has been introduced in the previous patches like 1.4.x. https://github.com/longhorn/longhorn/wiki/Deprecation-Policy

This issue is still pending in implement phase, @PhanLe1010 could you help with this to move this to ready-for-testing for QA? thanks.

@innobead
Copy link
Member

innobead commented May 29, 2023 •
edited
Loading

This is actually a duplicate of #5620. Let's close this for 1.5.0. This is valid for 1.4.x and 1.3.x, but invalid for 1.5.0.

@innobead innobead closed this as not planned Won't fix, can't repro, duplicate, stale May 29, 2023
@derekbit derekbit moved this to Closed in Longhorn Sprint Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PhanLe1010 PhanLe1010

Labels
backport/1.3.3 backport/1.4.2 kind/improvement Request for improvement of existing function priority/0 Must be implement or fixed in this release (managed by PO) require/doc Require updating the longhorn.io documentation wontfix
Projects
Longhorn Sprint
Status: Closed
Milestone
v1.5.0
Development

No branches or pull requests
4 participants
@innobead @yangchiu @PhanLe1010 @longhorn-io-github-bot