Skip to content

enhancement(stackit): alignment with other providers - #907

Open
Harsh-2005d wants to merge 1 commit into
flatcar:mainfrom
Harsh-2005d:stackit-fix
Open

enhancement(stackit): alignment with other providers#907
Harsh-2005d wants to merge 1 commit into
flatcar:mainfrom
Harsh-2005d:stackit-fix

Conversation

@Harsh-2005d

@Harsh-2005d Harsh-2005d commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Stackit: Alignment with other providers

Fixes- flatcar/Flatcar#2287

  • Make DeleteServer synchronous using wait.DeleteServerWaitHandler.
  • consistency of error handling across all deletion methods

Testing done

Build was successful.

  • Changelog entries added in the respective changelog/ directory (user-facing change, bug fix, security fix, update)
  • Inspected CI output for image differences: /boot and /usr size, packages, list files for any missing binaries, kernel modules, config files, kernel modules, etc.

@Harsh-2005d
Harsh-2005d requested a review from a team as a code owner August 4, 2026 14:40
@chewi

chewi commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I suspect @robinschneider deliberately did it with a retry for a reason.

@Harsh-2005d

Copy link
Copy Markdown
Contributor Author

@chewi I believe the retry was originally intended to account for the time it takes for the server to be deleted. Since DeleteServer now uses wait.DeleteServerWaitHandler to block until the resource is deleted, making the retry call redundant. Maybe @robinschneider can clarify more on it.

@robinschneider

Copy link
Copy Markdown
Member

@chewi I believe the retry was originally intended to account for the time it takes for the server to be deleted. Since DeleteServer now uses wait.DeleteServerWaitHandler to block until the resource is deleted, making the retry call redundant. Maybe @robinschneider can clarify more on it.

We should not remove the retry mechanism, OpenStack consists of multiple APIs, even if an API such as the one for server responds the server is already deleted, the other APIs might not yet know that and are not yet in sync.
This will cause problems if removed.

@Harsh-2005d
Harsh-2005d force-pushed the stackit-fix branch 3 times, most recently from f99d248 to e6709d2 Compare August 5, 2026 14:51
@Harsh-2005d

Copy link
Copy Markdown
Contributor Author

We should not remove the retry mechanism, OpenStack consists of multiple APIs, even if an API such as the one for server responds the server is already deleted, the other APIs might not yet know that and are not yet in sync. This will cause problems if removed.

Understood. I have restored the retry loop.

}

err := a.client.DeleteKeyPair(ctx, *keyPair.Name).Execute()
err := a.DeleteKeyPair(ctx, *keyPair.Name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You changed it here, but now gcNetworks, gcSecurityGroups and gcServers are inconsistent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated all the gc functions. Also wrapped the DeleteImage call and updated the error logs.

}
if isOpenAPINotFound(err) {
return nil, ErrorNotFound
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was not part of your change, but can you also align the order of the error cheks here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Make DeleteServer synchronous using `wait.DeleteServerWaitHandler`.
consistency of error handling across all deletion methods
Simplify garbage collection loops by implementing wrappers.

Signed-off-by: Harsh Dahiya <dahiyaharsh2005@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.

3 participants