Improve k8s status #563
Improve k8s status #563
Conversation
src/k8s/pkg/k8sd/database/sql/queries/feature-status/select.sql
Outdated
Show resolved
Hide resolved
|
@neoaggelos something that's bothering me is that we're not guaranteeing any consistency between I can think of two ways:
p.s. We can't really infer what do you think? |
|
Another question would be: In the case below, should we set the if !cfg.GetEnabled() {
if _, err := m.Apply(ctx, chartCalico, helm.StateDeleted, nil); err != nil {
applyErr := fmt.Errorf("failed to uninstall network: %w", err)
status.Message = fmt.Sprintf(deleteFailedMsgTmpl, applyErr)
status.Enabled = true // <-------- SHOULD BE HERE??
return status, applyErr
}
status.Version = ""
status.Message = disabledMsg
return status, nil
} |
So, we do not want to have people depending on The |
| # Verify output of the k8s status | ||
| result = instance.exec(["k8s", "status", "--wait-ready"], capture_output=True) | ||
| patterns = [ | ||
| r"cluster status:\s*ready", | ||
| r"control plane nodes:\s*(\d{1,3}(?:\.\d{1,3}){3}:\d{1,5})\s\(voter\)", | ||
| r"high availability:\s*no", | ||
| r"datastore:\s*k8s-dqlite", | ||
| r"network:\s*enabled", | ||
| r"dns:\s*enabled at (\d{1,3}(?:\.\d{1,3}){3})", | ||
| r"ingress:\s*enabled", | ||
| r"load-balancer:\s*enabled, Unknown mode", | ||
| r"local-storage:\s*enabled at /var/snap/k8s/common/rawfile-storage", | ||
| r"gateway\s*enabled", | ||
| ] | ||
| assert len(result.stdout.decode().strip().split("\n")) == len(patterns) | ||
|
|
||
| for i in range(len(patterns)): | ||
| timeout = 120 # seconds | ||
| t0 = time.time() | ||
| while ( | ||
| time.time() - t0 < timeout | ||
| ): # because some features might take time to get enabled | ||
| result_lines = ( | ||
| instance.exec(["k8s", "status", "--wait-ready"], capture_output=True) | ||
| .stdout.decode() | ||
| .strip() | ||
| .split("\n") | ||
| ) | ||
| line, pattern = result_lines[i], patterns[i] | ||
| if re.search(pattern, line) is not None: | ||
| break | ||
| LOG.info(f'Waiting for "{line}" to change...') | ||
| time.sleep(10) | ||
| else: | ||
| assert ( | ||
| re.search(pattern, line) is not None | ||
| ), f'"Wait timed out. {pattern}" not found in "{line}"' |
There was a problem hiding this comment.
this should use our stubbornly().until()
| enabledMsgTmpl = "enabled, %s mode" | ||
| disabledMsg = "disabled" | ||
| deleteFailedMsgTmpl = "Failed to delete MetalLB, the error was: %v" | ||
| deployFailedMsgTmpl = "Failed to deploy MetalLB, the error was: %v" |
There was a problem hiding this comment.
avoid mixing case, make all of these lowercase. also elsewhere.
also, these are fine to have a few times in the code below. reading fmt.Sprintf(deleteFailedMsgTmpl, err) does not make it easy to track where errors are coming from.
There was a problem hiding this comment.
I was trying to achieve the exact details in the proposal:
https://github.com/canonical/k8s-snap/blob/main/docs/proposals/001-k8s-status.md#:~:text=%22Failed%20to%20deploy%20Cilium%2C%20the%20error%20was%20%25w%22
There was a problem hiding this comment.
about the number of repetitions, I feel like having things like deleteFailedMsgTmpl does not hurt tracking down errors. because if I duplicate the error message in a file, not only it makes it so that changing that error is a bit harder (need to change a couple of places instead of one), and they might diverge (unwantedly), when you look up for an error you're faced with more than one result. I think it does more harm than good.
There was a problem hiding this comment.
and the somethingFailedMsgTmpl is supposed to be a message, not necessarily an error, so going with error best practices (with regards to mixed case) is not mandatory here I assume.
| ingressDeleteFailedMsgTmpl = "Failed to delete Contour Ingress, the error was: %v" | ||
| ingressDeployFailedMsgTmpl = "Failed to deploy Contour Ingress, the error was: %v" |
There was a problem hiding this comment.
same here, and other features as well
Reworked
k8s statusto comply with the new proposal