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

modify provision.sh to accept grpc port #708

Merged
merged 1 commit into from
May 26, 2023

Conversation

aryan9600
Copy link
Contributor

kind/bug

What this PR does / why we need it:
Modify provision.sh to accept a custom GRPC port using -p or --grpc-port. Currently we hardcode 9090 and provide no way to override it.

Fix a couple of return statements using the wrong error

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #568

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@Skarlso Skarlso added the kind/bug Something isn't working label May 16, 2023
hack/scripts/provision.sh Outdated Show resolved Hide resolved
hack/scripts/provision.sh Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.08 ⚠️

Comparison is base (22034dc) 58.26% compared to head (65bf099) 58.18%.

❗ Current head 65bf099 differs from pull request most recent head 4c65ca9. Consider uploading reports for the commit 4c65ca9 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #708      +/-   ##
==========================================
- Coverage   58.26%   58.18%   -0.08%     
==========================================
  Files          56       56              
  Lines        2705     2705              
==========================================
- Hits         1576     1574       -2     
- Misses        983      985       +2     
  Partials      146      146              
Impacted Files Coverage Δ
infrastructure/microvm/firecracker/create.go 0.00% <0.00%> (ø)
infrastructure/containerd/image_service.go 88.49% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Skarlso
Copy link
Contributor

Skarlso commented May 25, 2023

One last thing I would like to see if you don't mind @aryan9600 is an output of the config if you run this with and without the parameter is still consistent and looks okay. In the etc flintlock config file.

@aryan9600
Copy link
Contributor Author

❯ sudo ./hack/scripts/provision.sh flintlock -p 8080
❯ bat /etc/opt/flintlockd/config.yaml
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: /etc/opt/flintlockd/config.yaml
       │ Size: 136 B
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ ---
   2   │ insecure: false
   3   │ parent-iface: ens4
   4   │ grpc-endpoint: 10.190.0.12:8080
   5   │ verbosity: 9
   6   │ containerd-socket: /run/containerd/containerd.sock
   7   │ 
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
❯ sudo ./hack/scripts/provision.sh flintlock
❯ bat /etc/opt/flintlockd/config.yaml
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: /etc/opt/flintlockd/config.yaml
       │ Size: 136 B
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ ---
   2   │ insecure: false
   3   │ parent-iface: ens4
   4   │ grpc-endpoint: 10.190.0.12:9090
   5   │ verbosity: 9
   6   │ containerd-socket: /run/containerd/containerd.sock
   7   │ 
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

@Skarlso
Copy link
Contributor

Skarlso commented May 25, 2023

Fantastic, thank you.

Copy link
Contributor

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

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

LGTM

@richardcase would you like to add something else?

Modify provision.sh to accept a custom GRPC port using -p or
--grpc-port.
Fix a couple of return statements using the wrong error

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
@@ -169,7 +169,7 @@ func (im *imageService) snapshotAndMount(ctx context.Context,
logger.Debugf("image %s isn't unpacked, unpacking using %s snapshotter", image.Name(), snapshotter)

if unpackErr := image.Unpack(ctx, snapshotter); unpackErr != nil {
return nil, fmt.Errorf("unpacking %s with snapshotter %s: %w", image.Name(), snapshotter, err)
return nil, fmt.Errorf("unpacking %s with snapshotter %s: %w", image.Name(), snapshotter, unpackErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch

@@ -413,7 +418,7 @@ lookup_interface() {
lookup_address() {
local interface="$1"

ip route show | awk -v i="$interface" '$0 ~ i {print $9}' | grep -E '^(192\.168|10\.|172\.1[6789]\.|172\.2[0-9]\.|172\.3[01]\.)'
ip route show | awk -v i="$interface" '$0 ~ i {print $9}' | grep -E '^(192\.168|10\.|172\.1[6789]\.|172\.2[0-9]\.|172\.3[01]\.)' -m 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't even know why we didn't have the -m 1 option there before.

@@ -92,7 +92,7 @@ func (p *fcProvider) startFirecracker(cmd *exec.Cmd, vmState State, detached boo
}

if startErr != nil {
return nil, fmt.Errorf("starting firecracker process: %w", err)
return nil, fmt.Errorf("starting firecracker process: %w", startErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch

@yitsushi yitsushi enabled auto-merge (squash) May 25, 2023 22:24
@yitsushi yitsushi merged commit f421319 into liquidmetal-dev:main May 26, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provision.sh hardcodes 9090 as GRPC address
5 participants