Skip to content

Conversation

@sbernauer
Copy link
Member

@sbernauer sbernauer commented Aug 14, 2023

Description

Related to stackabletech/stackablectl#268

➜  stackable-cockpit git:(feat/check-demo-resoures) ✗ cargo r -p stackablectl -- demo in data-lakehouse-iceberg-trino-spark
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
     Running `target/debug/stackablectl demo in data-lakehouse-iceberg-trino-spark`
   WARN  The stack has the resource requirements [CPU: 71, Memory: 160Gi, PVC space: 1Ti], but cluster seems to have only 8 cores
    at rust/stackable-cockpit/src/platform/stack/spec.rs:138

   WARN  The stack has the resource requirements [CPU: 71, Memory: 160Gi, PVC space: 1Ti], but cluster seems to have only 47891.8Mi of memory
    at rust/stackable-cockpit/src/platform/stack/spec.rs:138

   WARN  The demo has the resource requirements [CPU: 80, Memory: 200Gi, PVC space: 1Ti], but cluster seems to have only 8 cores
    at rust/stackable-cockpit/src/platform/demo/spec.rs:97

   WARN  The demo has the resource requirements [CPU: 80, Memory: 200Gi, PVC space: 1Ti], but cluster seems to have only 47891.8Mi of memory
    at rust/stackable-cockpit/src/platform/demo/spec.rs:97

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes
# Author
- [ ] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Helm chart can be installed and deployed operator works
- [ ] Integration tests passed (for non trivial changes)
# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)
# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@sbernauer sbernauer changed the title feat: Warn when stack/demo is larget than cluster feat: Warn when stack/demo is larger than cluster Aug 14, 2023
@sbernauer sbernauer self-assigned this Aug 15, 2023
Copy link
Member

@Techassi Techassi left a comment

Choose a reason for hiding this comment

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

Some suggestions. Nice feature!

@sbernauer sbernauer requested a review from Techassi August 15, 2023 09:41
@Techassi
Copy link
Member

Some suggested improvements: feat/check-demo-resoures...feat/check-demo-resoures-techassi

@sbernauer
Copy link
Member Author

@Techassi did pull in your commit, but also push two other ones. Would be great if you could have a look!

@sbernauer sbernauer requested a review from Techassi August 16, 2023 11:27
Techassi
Techassi previously approved these changes Aug 17, 2023
Copy link
Member

@Techassi Techassi left a comment

Choose a reason for hiding this comment

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

LGTM. However we need to make sure that all pre-commits are run. Either manually like pre-commit run --all-files or by installing them permanently.

@nightkr
Copy link
Contributor

nightkr commented Aug 17, 2023

This feels a bit misleading, since it goes by a very optimistic view of scheduling (one node with no CPU but 1TiB RAM and another node with a 128-core EPYC and no RAM still wouldn't be able to schedule anything useful together).

I can see the value in giving early feedback, but I also wonder if it'd make more sense to focus on surfacing the actual scheduling errors in more discoverable ways.

@sbernauer
Copy link
Member Author

@Techassi thanks for review!
@nightkr good point! There might be cases where the warning is not shown but the demo does not run anyway.
I would argue that their is value in this feature already, as it resolves some issues where users tried to install demos on a too small k8s cluster, which this warning would have catched. We should not see this as an "If no warning is thrown, the demo will definitely run" but more as a "Let's handle 90% of the resource problems by printing a warning and not get asked the same question multiple times, which costs us time"

@sbernauer sbernauer requested review from Techassi and nightkr August 17, 2023 10:15
@nightkr
Copy link
Contributor

nightkr commented Aug 18, 2023

Yeah I don't want to block this PR on that.

@sbernauer
Copy link
Member Author

Alright, but it's a good and valid point!
Going forward I would like our stack and demo command waits for our product to be ready. In this place we can show the user the Pod events in the case the stack does not properly start. This could e.g. include 0/10 nodes sufficient memory etc.

@sbernauer sbernauer enabled auto-merge August 21, 2023 08:01
@sbernauer sbernauer added this pull request to the merge queue Aug 21, 2023
Merged via the queue into main with commit 5f77e0d Aug 21, 2023
@sbernauer sbernauer deleted the feat/check-demo-resoures branch August 21, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants