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

add drain_timeout_in_minutes and node_soak_duration_in_minutes #564

Merged

Conversation

zioproto
Copy link
Collaborator

Describe your changes

Implements the following GA functionalities:

Requires provider version v3.106.0
hashicorp/terraform-provider-azurerm#26137

Issue number

#530

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

@zioproto
Copy link
Collaborator Author

@morbendor
Copy link

Hey,

when the upcoming version of the module, which includes this changes, will be released?

Our CI/CD pipeline is dependent on that fix.

Thank you for your assistance.

@aditya-enthu
Copy link

aditya-enthu commented Jun 17, 2024

@zioproto can we consider adding upgrade config when auto-scaling is set to true:
https://github.com/zioproto/terraform-azurerm-aks/blob/feature/node-soak-duration-in-minutes/main.tf#L252

In addition, default value for agents_pool_drain_timeout_in_minutes is 30 min and setting it to null create resource to force-recreate.

upgrade_settings {
      - drain_timeout_in_minutes      = 30 -> null # forces replacement
      ~ node_soak_duration_in_minutes = 0 -> 10
        # (1 unchanged attribute hidden)
 }

while setting it to 30 min, or other values, it tries to update-in-place,

upgrade_settings {
      ~ drain_timeout_in_minutes      = 30 -> 40
      ~ node_soak_duration_in_minutes = 0 -> 10
        # (1 unchanged attribute hidden)
}

https://learn.microsoft.com/en-us/rest/api/aks/agent-pools/create-or-update?view=rest-aks-2024-02-01&tabs=HTTP#agentpoolupgradesettings

@zioproto
Copy link
Collaborator Author

when the upcoming version of the module, which includes this changes, will be released?

Because we have added two new required fields this will be released with version v10

@zioproto
Copy link
Collaborator Author

@aditya-enthu thanks for your review ! great feedback! I implemented your suggestions. Please have a look.

@aditya-enthu
Copy link

@aditya-enthu thanks for your review ! great feedback! I implemented your suggestions. Please have a look.

thank you for the quick fix, i tested locally and works fine.

@morbendor
Copy link

when the upcoming version of the module, which includes this changes, will be released?

Because we have added two new required fields this will be released with version v10

Thank you for the update. Could you please let us know the estimated release date for version v10, which includes the new required fields?

@zioproto
Copy link
Collaborator Author

Please @lonegunmanb could you comment on the release date ? thanks

@zioproto
Copy link
Collaborator Author

zioproto commented Jun 25, 2024

The end to end version upgrade test is failing :(
https://github.com/Azure/terraform-azurerm-aks/actions/runs/9641915202/job/26588582582?pr=564#step:6:247

It seems a problem with the default value of drain_timeout_in_minutes:

~ default_node_pool {
                    name                          = "nodepool"
                    tags                          = {}
                    # (33 unchanged attributes hidden)
        
                  ~ upgrade_settings {
                      ~ drain_timeout_in_minutes      = 0 -> 30
                        # (2 unchanged attributes hidden)
                    }
                }

@ms-henglu @lonegunmanb do you have any suggestion on how to activate this feature without recreating the cluster ?

Edit: after a closer look I don't see any "Force replacement" statement. I dont understand why the TestExampleUpgrade_without_monitor is failing. Can anyone help me understand why that step is failing ? thanks

@zioproto zioproto force-pushed the feature/node-soak-duration-in-minutes branch from 0ef0764 to d6d69df Compare June 26, 2024 07:13
Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants