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

fix: [sc-106256] Add missing uri field to troubleshoot.sh types #1578

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

nvanthao
Copy link
Member

@nvanthao nvanthao commented Jul 16, 2024

Story details: https://app.shortcut.com/replicated/story/106256
Demo: https://asciinema.org/a/c2tGPQrGZt6CM3jJQDZqKBsfv

This PR let preflight CLI load spec that has uri field, continued from #1574

I used Go reflect to extract all unique .Spec.uri in all troubleshoot objects. Once the PR is merged, we can perhaps update current logic in supportbundle CLI

// Load additional specs from support bundle URIs
// only when no-uri flag is not set and no URLs are provided in the args
if !viper.GetBool("no-uri") {
err := loadSupportBundleSpecsFromURIs(ctx, kinds)
if err != nil {
klog.Warningf("unable to load support bundles from URIs: %v", err)
}
}

@nvanthao nvanthao added the type::feature New feature or request label Jul 16, 2024
@nvanthao nvanthao requested a review from banjoh July 16, 2024 07:13
@nvanthao nvanthao self-assigned this Jul 16, 2024
@nvanthao nvanthao requested a review from a team as a code owner July 16, 2024 07:13
nvanthao added 2 commits July 16, 2024 17:15
* implement load additional spec from URIs
* add unit tests
@nvanthao nvanthao force-pushed the gerard/sc-106256/load-preflight-uris branch from dba164b to b6fda31 Compare July 16, 2024 07:15
Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

When I tested this out, I could not replace the entire spec with what got downloaded from a uri. Instead, the specs get merged together

Original spec

apiVersion: troubleshoot.sh/v1beta2
kind: HostPreflight
metadata:
  name: my-spec
spec:
  uri: https://gist.githubusercontent.com/banjoh/142fabe108e199d42d442cc0017c7064/raw/a75ceff7f1ec40b58fc38c0dde70a20e9453b7f0/filesystem-preflight.yaml
  collectors:
  - diskUsage:
      collectorName: var-lib-kubelet
      path: /var/lib/kubelet

Wrongly updated spec

apiVersion: troubleshoot.sh/v1beta2
kind: HostPreflight
metadata:
  creationTimestamp: null
  name: my-spec
spec:
  analyzers:
  - filesystemPerformance:
      collectorName: Filesystem latency check
      outcomes:
      - pass:
          message: 'Write latency is ok (p99 target < 10ms, actual: {{ .P99 }})'
          when: p99 < 10ms
      - fail:
          message: Write latency is high. p99 target < 10ms, actual:{{ .String }}
  collectors:
  - diskUsage:
      collectorName: var-lib-kubelet
      path: /var/lib/kubelet
  - filesystemPerformance:
      backgroundIOPSWarmupSeconds: 10
      backgroundReadIOPS: 50
      backgroundReadIOPSJobs: 1
      backgroundWriteIOPS: 300
      backgroundWriteIOPSJobs: 6
      collectorName: Filesystem Latency Two Minute Benchmark
      datasync: true
      directory: /var/lib/etcd
      enableBackgroundIOPS: true
      fileSize: 22Mi
      timeout: 2m
  uri: https://gist.githubusercontent.com/banjoh/142fabe108e199d42d442cc0017c7064/raw/a75ceff7f1ec40b58fc38c0dde70a20e9453b7f0/filesystem-preflight.yaml
status: {}

Expected spec update

apiVersion: troubleshoot.sh/v1beta2
kind: HostPreflight
metadata:
  name: fs-preflight
spec:
  collectors:
    - filesystemPerformance:
        collectorName: Filesystem Latency Two Minute Benchmark
        timeout: 2m
        directory: /var/lib/etcd
        fileSize: 22Mi
        operationSizeBytes: 2300
        datasync: true
        enableBackgroundIOPS: true
        backgroundIOPSWarmupSeconds: 10
        backgroundWriteIOPS: 300
        backgroundWriteIOPSJobs: 6
        backgroundReadIOPS: 50
        backgroundReadIOPSJobs: 1
  analyzers:
    - filesystemPerformance:
        collectorName: Filesystem latency check
        outcomes:
          - pass:
              when: "p99 < 10ms"
              message: "Write latency is ok (p99 target < 10ms, actual: {{ .P99 }})"
          - fail:
              message: "Write latency is high. p99 target < 10ms, actual:{{ .String }}"

@nvanthao
Copy link
Member Author

Hi @banjoh,

I've made changes to the code to follow our design doc. That is, to replace the original one entirely, not merge

Updated demo: https://asciinema.org/a/2n6nrYBIByC6SIuTsTSInX1sC

Please let me know how it goes.

@nvanthao nvanthao requested a review from banjoh July 18, 2024 03:59
@nvanthao nvanthao merged commit 04e656a into main Jul 18, 2024
27 checks passed
@nvanthao nvanthao deleted the gerard/sc-106256/load-preflight-uris branch July 18, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants