fix(BRE2-940): out of Nebius capacity error mapping and ufw regression#118
Conversation
f49162d to
ec6ab83
Compare
287dc95 to
d684d75
Compare
1c472db to
590764d
Compare
| var serviceErr *serviceerror.Error | ||
| if errors.As(e, &serviceErr) { | ||
| for _, detail := range serviceErr.Details { | ||
| switch detail.(type) { | ||
| case *serviceerror.NotEnoughResources: | ||
| return v1.ErrInsufficientResources | ||
| case *serviceerror.QuotaFailure: | ||
| return v1.ErrOutOfQuota | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice. I wonder if the below ever actually fire? Interesting that we are looking directly at grpc codes there, whereas here we are testing the err type (which seems more appropriate and expected).
There was a problem hiding this comment.
ya I was poking through the code and was similarly confused by the casting. It seems there are a couple paths and the errors get rewrapped as serviceerror or operations. I figured no harm if this block is the capture now. Though I am wondering if the below block was just always wrong.
| // UFW persists its own rules in /etc/ufw; only DOCKER-USER needed a Docker | ||
| // startup hook after removing netfilter-persistent. |
There was a problem hiding this comment.
This is probably going to be confusing to future readers as it is mainly documenting what changed from in this PR ("after removing netfilter-persistent").
| // DOCKER-USER is Docker's documented filter hook for this traffic. The | ||
| // ordering is important: some Nebius images run cloud-init before Docker has | ||
| // created DOCKER-USER, and Docker may create/reset the chain during daemon | ||
| // startup. We therefore install both: | ||
| // - an immediate cloud-init run for images where Docker is already active | ||
| // - a docker.service ExecStartPost hook for images where Docker starts later |
There was a problem hiding this comment.
From cloudinit logs (which we should start looking at and triggering a build failure for if they do not succeed):
...
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
iptables: No chain/target/match by that name.
...
it seems like the core fix should be to add an initial line in the iptables setup to ensure the chain is created (docker itself doesn't need to be the one to create it):
iptables -N DOCKER-USER || true
it looks like the new firewall script does exactly this -- so the comment here isn't wrong to mention ordering, but it could be simplified to state "we need to ensure the chain exists so we will attempt to create it, but it's possible that this is running after docker itself created it in which case we will ignore the error and move on".
There was a problem hiding this comment.
Actually on that note:
https://github.com/brevdev/cloud/blob/main/v1/providers/shadeform/firewall.go#L19
Could you add a "iptables -N DOCKER-USER || true" on the shadeform side? Better safe than sorry.
| dockerServiceDropInDir = "/etc/systemd/system/docker.service.d" | ||
| dockerFirewallDropInPath = dockerServiceDropInDir + "/10-brev-firewall.conf" |
There was a problem hiding this comment.
This one actually might be worth a comment as we alternatively could make a oneshot unit.
The benefit of a standalone unit is that we can inspect it individually, so we could do something like systemctl status brev-docker-firewall. With this we'll need to just go straight to the docker service (e.g. journalctl docker. Not necessarily a bad thing, but worth considering if/when we need to answer the question "what happened to this VM's iptables?"
3381e85 to
1fbbb2b
Compare
NotEnoughResourcesservice errors toErrInsufficientResourcesinstead of treating them as quota failures.For capacity errors, Nebius can wrap
NotEnoughResourcesinside aserviceerror.Error/ operation error. We now inspect the service-error details before falling back to genericResourceExhaustedhandling, so true capacity exhaustion is classified correctly while quota failures still map toErrOutOfQuota. ExampleFor firewalling, some Nebius CPU images run cloud-init before Docker creates the
DOCKER-USERchain. The old setup tried to writeDOCKER-USERrules too early, Docker later started with an empty chain, anddocker run -p ...ports became publicly reachable. This change installs an idempotent Docker firewall script and runs it both during cloud-init and via adocker.serviceExecStartPosthook.The validation test has been failing for weeks because we now use a CPU image in our test pipeline, not a GPU one.