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

Set ordered names for guest devices #344

Merged
merged 5 commits into from
Jan 6, 2022

Conversation

yitsushi
Copy link
Contributor

@yitsushi yitsushi commented Jan 5, 2022

What this PR does / why we need it:

  • We will rely on the order specified in the incoming grpc request and
    the fact that serialization in go shouldn't change the order
  • We will update flintlock docs to say that if you care about the order
    of your network interfaces then you must specify them in the order
    from eth1,eth2,ethX
  • MMDS device is the first (eth0)

There is no relation between the numbering of the /network-interface
API calls and the number of the network interface in the guest. Rather,
it is usually the order of network interface creation that determines
the number in the guest (but this depends on the distribution).

For example, when you create two network interfaces by calling
/network-interfaces/1 and then /network-interfaces/0, it may result in
this mapping:

/network-interfaces/1 -> eth0
/network-interfaces/0 -> eth1

Source: https://github.com/firecracker-microvm/firecracker/blob/83ec483d4254936e1e2fd4adfe94761611a74a93/FAQ.md#how-does-network-interface-numbering-work

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 #340

Special notes for your reviewer:

Checklist:

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

@yitsushi yitsushi added the kind/feature New feature or request label Jan 5, 2022
* We will rely on the order specified in the incoming grpc request and
  the fact that serialization in go shouldn't change the order
* We will update flintlock docs to say that if you care about the order
  of your network interfaces then you must specify them in the order
  from eth1,eth2,ethX
* MMDS device is the first (eth0)

> There is no relation between the numbering of the /network-interface
> API calls and the number of the network interface in the guest. Rather,
> it is usually the order of network interface creation that determines
> the number in the guest (but this depends on the distribution).
>
> For example, when you create two network interfaces by calling
> /network-interfaces/1 and then /network-interfaces/0, it may result in
> this mapping:
>
>    /network-interfaces/1 -> eth0
>    /network-interfaces/0 -> eth1

Source: https://github.com/firecracker-microvm/firecracker/blob/83ec483d4254936e1e2fd4adfe94761611a74a93/FAQ.md#how-does-network-interface-numbering-work

Fixes liquidmetal-dev#340
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #344 (e6a47a3) into main (7a6932e) will increase coverage by 0.11%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
+ Coverage   58.09%   58.20%   +0.11%     
==========================================
  Files          51       51              
  Lines        2503     2505       +2     
==========================================
+ Hits         1454     1458       +4     
+ Misses        932      930       -2     
  Partials      117      117              
Impacted Files Coverage Δ
infrastructure/grpc/convert.go 41.80% <66.66%> (+0.66%) ⬆️
infrastructure/grpc/defaults.go 100.00% <100.00%> (ø)
infrastructure/containerd/event_service.go 59.64% <0.00%> (+3.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a6932e...e6a47a3. Read the comment docs.

Callisto13
Callisto13 previously approved these changes Jan 6, 2022
Copy link
Member

@Callisto13 Callisto13 left a comment

Choose a reason for hiding this comment

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

2 tiny typos but otherwise lgtm

api/types/microvm.proto Outdated Show resolved Hide resolved
api/types/microvm.proto Outdated Show resolved Hide resolved
yitsushi and others added 2 commits January 6, 2022 12:11
Callisto13
Callisto13 previously approved these changes Jan 6, 2022
@Callisto13
Copy link
Member

actually you'll probably also need to regenerate something after changing those comments

@yitsushi yitsushi merged commit e5aa3b8 into liquidmetal-dev:main Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set ordered names for guest devices
3 participants