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

feat: Added new option enableProvidedByTopology #780

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

lukasmetzner
Copy link
Contributor

We are reintroducing a feature originally present in v2.10.0 to prevent pods from getting stuck in the pending state in clusters with non-cloud nodes. This feature is now optional and can be enabled via the Helm Chart. By default, it remains disabled to avoid compatibility issues with Nomad clusters, which have a different CSI spec implementation.

Learn more about it in #400.

@lukasmetzner lukasmetzner requested a review from a team as a code owner November 12, 2024 14:48
@lukasmetzner lukasmetzner self-assigned this Nov 12, 2024
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 53.65854% with 19 lines in your changes missing coverage. Please review.

Project coverage is 35.85%. Comparing base (a9d8505) to head (3f62d81).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/app/app.go 0.00% 6 Missing ⚠️
cmd/aio/main.go 0.00% 4 Missing ⚠️
cmd/controller/main.go 0.00% 3 Missing ⚠️
cmd/node/main.go 0.00% 3 Missing ⚠️
internal/driver/node.go 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #780      +/-   ##
==========================================
- Coverage   35.95%   35.85%   -0.10%     
==========================================
  Files          20       20              
  Lines        1847     1874      +27     
==========================================
+ Hits          664      672       +8     
- Misses       1150     1168      +18     
- Partials       33       34       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@lukasmetzner lukasmetzner force-pushed the feat-enable-provided-by-topology branch from 41a3603 to 4f06570 Compare November 12, 2024 15:07
@resmo
Copy link
Contributor

resmo commented Nov 15, 2024

sorry for the question here but as a nomad user, we probably also want make use of this feature.

So I briefly looked through some docs and this is what I found:

On nomad client nodes, we can define meta info:

client {
  enabled       = true
  node_class    = "prod"

  meta {
    owner           = "ops"
    rack            = "rack-12-1"
  }
}

Which seems to be used in ebs csi plugin

topology_request {
  required {
    topology { segments { rack = "R1", zone = "us-east-1a" } }
    topology { segments { rack = "R2", zone = "us-east-1a" } }
  }
  preferred {
    topology { segments { rack = "R1", zone = "us-east-1a"} }
  }
}

that is why I tried this to check if this would work with v2.10.0, but it didn't but, but shouldn't it? Is there maybe just a small mistake somewhere?

client {
  enabled = true

  meta {
    "instance.hetzner.cloud/provided-by" = "cloud"
  }

@lukasmetzner
Copy link
Contributor Author

@resmo I don't have much Nomad knowledge yet, but it might be enough to specify an anti-affinity towards the non-cloud nodes on the CSI Node Job.

This should also suffice in Kubernetes, but there is an upstream issue in the scheduler, which we built this workaround for.

We are reintroducing a feature originally present in v2.10.0 to prevent pods from getting stuck in the `pending` state in clusters with non-cloud nodes. This feature is now optional and can be enabled via the Helm Chart. By default, it remains disabled to avoid compatibility issues with Nomad clusters, which have a different CSI spec implementation.
@lukasmetzner lukasmetzner force-pushed the feat-enable-provided-by-topology branch from 0cab3aa to 3f62d81 Compare November 18, 2024 15:24
@lukasmetzner lukasmetzner merged commit 7e72431 into main Nov 19, 2024
8 checks passed
@lukasmetzner lukasmetzner deleted the feat-enable-provided-by-topology branch November 19, 2024 11:11
lukasmetzner pushed a commit that referenced this pull request Nov 26, 2024
<!-- section-start changelog -->
### Features

- Added new option enableProvidedByTopology (#780)
- drop tests for kubernetes v1.28 (#796)

### Bug Fixes

- prefer scheduling the csi controller on cloud nodes (#786)

### Kubernetes Support

This version was tested with Kubernetes 1.29 - 1.31. Furthermore, we
dropped v1.28 support.

<!-- section-end changelog -->

---

<details>
<summary><h4>PR by <a
href="https://github.com/apricote/releaser-pleaser">releaser-pleaser</a>
🤖</h4></summary>

If you want to modify the proposed release, add you overrides here. You
can learn more about the options in the docs.

## Release Notes

### Prefix / Start

This will be added to the start of the release notes.

```rp-prefix
```

### Suffix / End

This will be added to the end of the release notes.

```rp-suffix
### Kubernetes Support

This version was tested with Kubernetes 1.29 - 1.31. Furthermore, we dropped v1.28 support.
```

</details>

Co-authored-by: releaser-pleaser <>
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