Deuxfleurs/garage
41
56
Fork 73
Code Issues 98 Pull requests 14 Releases 30 Wiki Activity

replace RPC stack with netapp #123

Merged
lx merged 5 commits from netapp into main 2021-10-25 13:49:35 +00:00
Conversation 2 Commits 5 Files changed 53 +4011 -4621
lx commented 2021-10-14 15:16:46 +00:00
Owner
Copy link

NOTICE

  • this change removes the migration path from the v0.2 series
  • configuration/setup is vastly changed, please refer to new documentation

TODO

  • Update documentation section on config file reference
  • Persist peer list in local file
  • Netapp: test priority queue (it's probably wrong)
  • Consul advertisement that includes node's public key in NodeMeta
  • Check if we can NAT with a different port thanks to rpc_public_addr
  • Try a migration from 0.3 to 0.4
  • Document (at least roughly) migration path from 0.3 to 0.4
NOTICE - this change removes the migration path from the `v0.2` series - configuration/setup is vastly changed, please refer to new documentation TODO - [x] Update documentation section on config file reference - [x] Persist peer list in local file - [x] Netapp: test priority queue (it's probably wrong) - [x] Consul advertisement that includes node's public key in `NodeMeta` - [ ] Check if we can NAT with a different port thanks to `rpc_public_addr` - [x] Try a migration from 0.3 to 0.4 - [x] Document (at least roughly) migration path from 0.3 to 0.4
lx changed title from WIP replace RPC stack with netapp to WIP: replace RPC stack with netapp 2021-10-18 13:28:01 +00:00
lx force-pushed netapp from 8ae40088d9 to 97c5d5c77f 2021-10-19 12:35:54 +00:00 Compare
lx force-pushed netapp from 4a555c0c7d to d9c52e9a9c 2021-10-19 21:40:42 +00:00 Compare
lx added 1 commit 2021-10-19 21:53:58 +00:00
Fix clippy
Some checks failed
continuous-integration/drone/push Build is failing
Details
continuous-integration/drone/pr Build is failing
Details
f1a68f6b57
lx added 2 commits 2021-10-19 22:00:42 +00:00
run cargo2nix
All checks were successful
continuous-integration/drone/push Build is passing
Details
continuous-integration/drone/pr Build is passing
Details
c18920f8d1
quentin reviewed 2021-10-20 07:31:44 +00:00
doc/book/src/cookbook/real_world.md Outdated
@ -127,0 +110,4 @@
563e1ac825ee3323aa441e72c26d1030d6d4414aeb3dd25287c531e7fc2bc95d@[fc00:1::1]:3901
Venus$ garage node-id
86f0f26ae4afbd59aaf9cfb059eefac844951efd5b8caeec0d53f4ed6c85f332[fc00:1::2]:3901
quentin commented 2021-10-20 07:31:44 +00:00
Owner
Copy link

Maybe a missing @ here?

Maybe a missing @ here?
quentin reviewed 2021-10-20 07:31:54 +00:00
doc/book/src/cookbook/real_world.md Outdated
@ -127,0 +120,4 @@
```toml
bootstrap_peers = [
"563e1ac825ee3323aa441e72c26d1030d6d4414aeb3dd25287c531e7fc2bc95d@[fc00:1::1]:3901",
"86f0f26ae4afbd59aaf9cfb059eefac844951efd5b8caeec0d53f4ed6c85f332[fc00:1::2]:3901",
quentin commented 2021-10-20 07:31:54 +00:00
Owner
Copy link

idem here?

idem here?
quentin reviewed 2021-10-20 07:37:58 +00:00
script/dev-cluster.sh Outdated
@ -64,1 +65,3 @@
(garage server -c /tmp/config.$count.toml 2>&1|while read r; do echo -en "$LABEL $r\n"; done) &
(garage -c /tmp/config.$count.toml server 2>&1|while read r; do echo -en "$LABEL $r\n"; done) &
done
# >>>>>>>>>>>>>>>> END FOR LOOP ON NODES
quentin commented 2021-10-20 07:37:58 +00:00
Owner
Copy link

check socat multiple launch. Tracked in #124

check socat multiple launch. Tracked in #124
lx added 1 commit 2021-10-20 14:10:29 +00:00
Update documentation
All checks were successful
continuous-integration/drone/push Build is passing
Details
continuous-integration/drone/pr Build is passing
Details
6455347955
lx added 1 commit 2021-10-20 22:30:31 +00:00
Discovery via consul
All checks were successful
continuous-integration/drone/push Build is passing
Details
continuous-integration/drone/pr Build is passing
Details
5e986c443f
lx force-pushed netapp from 832602d7f3 to 1dc7cc7936 2021-10-21 10:34:10 +00:00 Compare
lx added 1 commit 2021-10-21 11:46:30 +00:00
Persist peer list to file
All checks were successful
continuous-integration/drone/push Build is passing
Details
continuous-integration/drone/pr Build is passing
Details
2fa65554f1
quentin commented 2021-10-21 12:05:02 +00:00
Owner
Copy link

you need to run cargo2nix -f again after updating netapp :)

you need to run `cargo2nix -f` again after updating netapp :)
lx added 1 commit 2021-10-21 12:10:20 +00:00
update cargo.nix
All checks were successful
continuous-integration/drone/push Build is passing
Details
continuous-integration/drone/pr Build is passing
Details
b7eccf5264
quentin approved these changes 2021-10-21 13:21:48 +00:00
quentin left a comment
Owner
Copy link

I have reviewed your merge request and agree to merge it.

Before merging it, I think our main requirement is to make garage node-id works when the meta directory has not been created yet (please refer to my comment).

It seems to be an already existing bug but we may display an empty failed node list in some cases that is very useless when debugging (please refer to my comment). We must at least track this.

Might be for a future PR but we should improve error messages, especially when:

  • a file or folder does not exist
  • when the rpc_secret does not match
  • when we failed to meet the quorum
  • when the user put a node url with the wrong format

Might also be for a future PR but the secret handshake protocol will be a bit new for our users, we should write some documentation about securing a cluster. We must mention:

  • rpc_secret
  • node_id
  • firewalls
  • (hardened systemd service)

I have not:

  • tested Consul
  • done performance tests

If you are not confident on one of these two points, I can test them before you merge.

I have reviewed your merge request and agree to merge it. Before merging it, I think our main requirement is to make `garage node-id` works when the `meta` directory has not been created yet (please refer to my comment). It seems to be an already existing bug but we may display an empty failed node list in some cases that is very useless when debugging (please refer to my comment). We must at least track this. Might be for a future PR but we should improve error messages, especially when: - a file or folder does not exist - when the `rpc_secret` does not match - when we failed to meet the quorum - when the user put a node url with the wrong format Might also be for a future PR but the secret handshake protocol will be a bit new for our users, we should write some documentation about securing a cluster. We must mention: - rpc_secret - node_id - firewalls - (hardened systemd service) I have not: - tested Consul - done performance tests If you are not confident on one of these two points, I can test them before you merge.
src/garage/cli.rs Outdated
@ -391,3 +388,2 @@
if failure_case_1 || failure_case_2 {
println!("\nFailed nodes:");
for adv in status.iter().filter(|x| !x.is_up) {
println!("\n==== FAILED NODES ====");
quentin commented 2021-10-21 09:08:35 +00:00
Owner
Copy link

If we are on failure_case_2 we display the failed nodes section but it is empty.

It can be reproduced by spanning 2 garage instances then connecting the first instance to the second one.

eg:

$ ./result/bin/garage -c /tmp/garage.toml status
==== HEALTHY NODES ====
ID                 Hostname     Address                  Tag               Zone  Capacity
2f75a8a915f9cceb…  lheureduthe  127.0.0.1:3911           NO ROLE ASSIGNED
c393f0ab78d8ddcd…  lheureduthe  [::ffff:127.0.0.1]:3901  NO ROLE ASSIGNED

$ ./result/bin/garage status
==== HEALTHY NODES ====
ID                 Hostname     Address         Tag               Zone  Capacity
c393f0ab78d8ddcd…  lheureduthe  127.0.0.1:3901  NO ROLE ASSIGNED

==== FAILED NODES ====
ID  Hostname  Address  Tag  Zone  Capacity  Last seen
If we are on `failure_case_2` we display the failed nodes section but it is empty. It can be reproduced by spanning 2 garage instances then connecting the first instance to the second one. eg: ``` $ ./result/bin/garage -c /tmp/garage.toml status ==== HEALTHY NODES ==== ID Hostname Address Tag Zone Capacity 2f75a8a915f9cceb… lheureduthe 127.0.0.1:3911 NO ROLE ASSIGNED c393f0ab78d8ddcd… lheureduthe [::ffff:127.0.0.1]:3901 NO ROLE ASSIGNED $ ./result/bin/garage status ==== HEALTHY NODES ==== ID Hostname Address Tag Zone Capacity c393f0ab78d8ddcd… lheureduthe 127.0.0.1:3901 NO ROLE ASSIGNED ==== FAILED NODES ==== ID Hostname Address Tag Zone Capacity Last seen ```
src/garage/main.rs Outdated
@ -80,0 +97,4 @@
// Find and parse the address of the target host
let (id, addr) = if let Some(h) = opt.rpc_host {
let (id, addrs) = parse_and_resolve_peer_addr(&h).expect("Invalid RPC host");
quentin commented 2021-10-20 13:47:47 +00:00
Owner
Copy link

We should replace this error message by something like that:

Invalid remote peer identifier "127.0.0.1:3911".
A remote peer identifier has the following format: <pubkey>@<address>:<port>.
Example: 9286ef3c3127b199d4a28928a261584f772dd674fc0aab9860d83a1849d3fb45@127.0.0.1:3901

The most important in my opinion is to drop the "rpc host" and "host" terminology in favor of a more abstract terminology such as "remote peer identifier" that can not be confused with the layer 3 or layer 4 terminology/patterns.

We should replace this error message by something like that: ``` Invalid remote peer identifier "127.0.0.1:3911". A remote peer identifier has the following format: <pubkey>@<address>:<port>. Example: 9286ef3c3127b199d4a28928a261584f772dd674fc0aab9860d83a1849d3fb45@127.0.0.1:3901 ``` The most important in my opinion is to drop the "rpc host" and "host" terminology in favor of a more abstract terminology such as "remote peer identifier" that can not be confused with the layer 3 or layer 4 terminology/patterns.
src/garage/main.rs Outdated
@ -98,0 +144,4 @@
format!("{}@127.0.0.1:3901", idstr)
};
if !quiet {
quentin commented 2021-10-20 13:29:10 +00:00
Owner
Copy link

I think we can improve the wording of this very helpful and critical message :)

  1. We speak about "nodes" indiferrently, we can qualify them as "remote" or "local", "existing" or "new", etc. to help the user identifiy the new/local node he/she is configuring and the remote/existing nodes he/she has already configured.

  2. We could better separate concerns (on this node // on a cluster node, your cluster is live // you are configuring it) and prioritize our examples.

  3. Does it mean that we can join any cluster? If yes, this is a secrity issue.

I will propose an alternative text after gaining some more knowledge on this process.

I think we can improve the wording of this *very helpful and critical message* :) 1. We speak about "nodes" indiferrently, we can qualify them as "remote" or "local", "existing" or "new", etc. to help the user identifiy the new/local node he/she is configuring and the remote/existing nodes he/she has already configured. 2. We could better separate concerns (on this node // on a cluster node, your cluster is live // you are configuring it) and prioritize our examples. 3. Does it mean that we can join any cluster? If yes, this is a secrity issue. I will propose an alternative text after gaining some more knowledge on this process.
src/garage/main.rs Outdated
@ -98,0 +158,4 @@
idstr
);
eprintln!(
"where <remote_node> is their own node identifier in the format: <pubkey>@<ip>:<port>"
quentin commented 2021-10-20 13:43:26 +00:00
Owner
Copy link

Ok, I missed this point the first time I tested, I tried with 127.0.0.1:3911 and got the following error:

thread 'main' panicked at 'Invalid RPC host', main.rs:100:59

I think here we should directly put <pubkey>@<address>:<port> instead of <remote_node>.

Ok, I missed this point the first time I tested, I tried with `127.0.0.1:3911` and got the following error: ``` thread 'main' panicked at 'Invalid RPC host', main.rs:100:59 ``` I think here we should directly put `<pubkey>@<address>:<port>` instead of `<remote_node>`.
src/rpc/system.rs Outdated
@ -0,0 +128,4 @@
} else {
let (_, key) = ed25519::gen_keypair();
let mut f = std::fs::File::create(key_file.as_path())?;
quentin commented 2021-10-20 13:17:10 +00:00
Owner
Copy link
  1. Create a file /etc/garage.toml with the content given in the Quickstart
  2. Never start the daemon (check that no meta or data folder have been created)
  3. Run garage node-id (this is similar to the steps advertised in "Cookbook > Deploying Garage"
  4. Get an error:
strace -e open ./result/bin/garage node-id
open("/proc/self/cgroup", O_RDONLY|O_CLOEXEC) = 3
open("/proc/self/mountinfo", O_RDONLY|O_CLOEXEC) = 3
open("/sys/fs/cgroup/cpu,cpuacct/user.slice/user-1000.slice/user@1000.service/cpu.cfs_quota_us", O_RDONLY|O_CLOEXEC) = 3
open("/etc/garage.toml", O_RDONLY|O_CLOEXEC) = 9
open("/tmp/meta/node_key", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = -1 ENOENT (Aucun fichier ou dossier de ce type)
 ERROR garage > Unable to read or generate node key: IO error: No such file or directory (os error 2)
+++ exited with 1 +++

2 points:

  1. We need to recursively create folders before trying to write the key
  2. We might want to improve our error reporting by specifying the failed path.
1. Create a file `/etc/garage.toml` with the content given in the Quickstart 2. Never start the daemon (check that no meta or data folder have been created) 3. Run `garage node-id` (this is similar to the steps advertised in "Cookbook > Deploying Garage" 4. Get an error: ``` strace -e open ./result/bin/garage node-id open("/proc/self/cgroup", O_RDONLY|O_CLOEXEC) = 3 open("/proc/self/mountinfo", O_RDONLY|O_CLOEXEC) = 3 open("/sys/fs/cgroup/cpu,cpuacct/user.slice/user-1000.slice/user@1000.service/cpu.cfs_quota_us", O_RDONLY|O_CLOEXEC) = 3 open("/etc/garage.toml", O_RDONLY|O_CLOEXEC) = 9 open("/tmp/meta/node_key", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = -1 ENOENT (Aucun fichier ou dossier de ce type) ERROR garage > Unable to read or generate node key: IO error: No such file or directory (os error 2) +++ exited with 1 +++ ``` 2 points: 1. We need to recursively create folders before trying to write the key 2. We might want to improve our error reporting by specifying the failed path.
src/rpc/system.rs Outdated
@ -0,0 +130,4 @@
} else {
let (_, key) = ed25519::gen_keypair();
let mut f = std::fs::File::create(key_file.as_path())?;
quentin commented 2021-10-21 13:10:11 +00:00
Owner
Copy link
$ RUST_LOG=garage=debug,netapp=debug strace -e open ./result/bin/garage node-id
open("/proc/self/cgroup", O_RDONLY|O_CLOEXEC) = 3
open("/proc/self/mountinfo", O_RDONLY|O_CLOEXEC) = 3
open("/sys/fs/cgroup/cpu,cpuacct/user.slice/user-1000.slice/user@1000.service/cpu.cfs_quota_us", O_RDONLY|O_CLOEXEC) = 3
open("/etc/garage.toml", O_RDONLY|O_CLOEXEC) = 9
open("/tmp/meta/node_key", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = -1 ENOENT (Aucun fichier ou dossier de ce type)
 ERROR garage > Unable to read or generate node key: IO error: No such file or directory (os error 2)
+++ exited with 1 +++

There are many chance that the garage node-id command will fail as the key will be stored in the meta folder that will very likely not be created yet and it will throw the previous cryptic error I a diagnosed through strace.

``` $ RUST_LOG=garage=debug,netapp=debug strace -e open ./result/bin/garage node-id open("/proc/self/cgroup", O_RDONLY|O_CLOEXEC) = 3 open("/proc/self/mountinfo", O_RDONLY|O_CLOEXEC) = 3 open("/sys/fs/cgroup/cpu,cpuacct/user.slice/user-1000.slice/user@1000.service/cpu.cfs_quota_us", O_RDONLY|O_CLOEXEC) = 3 open("/etc/garage.toml", O_RDONLY|O_CLOEXEC) = 9 open("/tmp/meta/node_key", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = -1 ENOENT (Aucun fichier ou dossier de ce type) ERROR garage > Unable to read or generate node key: IO error: No such file or directory (os error 2) +++ exited with 1 +++ ``` There are many chance that the `garage node-id` command will fail as the key will be stored in the `meta` folder that will very likely not be created yet and it will throw the previous cryptic error I a diagnosed through strace.
src/util/error.rs Outdated
@ -50,3 +26,1 @@
#[error(display = "PKI error: {}", _0)]
Pki(#[error(source)] webpki::Error),
#[error(display = "Netapp error: {}", _0)]
quentin commented 2021-10-20 16:20:48 +00:00
Owner
Copy link

When the wrong rpc_secret is used and that it leads to a failed handshake, we should add a hint to the user that it may check his/her secret key.

When the wrong `rpc_secret` is used and that it leads to a failed handshake, we should add a hint to the user that it may check his/her secret key.
src/util/error.rs Outdated
@ -71,3 +49,2 @@
#[error(display = "Remote error: {} (status code {})", _0, _1)]
RemoteError(String, StatusCode),
#[error(display = "Too many errors: {:?}", _0)]
quentin commented 2021-10-20 13:09:23 +00:00
Owner
Copy link

We discussed renaming this error "FailedQuorumError" or something similar as this is the only case it is fired.

We discussed renaming this error "FailedQuorumError" or something similar as this is the only case it is fired.
quentin commented 2021-10-21 15:23:49 +00:00
Owner
Copy link

Fixes #101
Fixes #36

Fixes #101 Fixes #36
lx added 1 commit 2021-10-21 15:29:45 +00:00
Move things around, improvements to CLI
All checks were successful
continuous-integration/drone/push Build is passing
Details
continuous-integration/drone/pr Build is passing
Details
bff5333c62
lx requested review from quentin 2021-10-21 15:42:20 +00:00
lx force-pushed netapp from 0a45a2b8ee to dbe457d3fa 2021-10-21 15:43:48 +00:00 Compare
lx changed title from WIP: replace RPC stack with netapp to replace RPC stack with netapp 2021-10-22 13:35:43 +00:00
lx force-pushed netapp from d838d604eb to f4d246ee24 2021-10-22 13:48:07 +00:00 Compare
lx force-pushed netapp from f4d246ee24 to 991fe2032f 2021-10-22 13:50:59 +00:00 Compare
lx force-pushed netapp from 991fe2032f to 2a1dc24710 2021-10-22 13:53:51 +00:00 Compare
lx force-pushed netapp from 2a1dc24710 to db2924ad80 2021-10-22 13:55:51 +00:00 Compare
lx force-pushed netapp from db2924ad80 to 28c3c27c26 2021-10-22 13:59:07 +00:00 Compare
lx force-pushed netapp from 28c3c27c26 to 2eba3d6d62 2021-10-22 14:52:34 +00:00 Compare
lx force-pushed netapp from 2eba3d6d62 to 6d8b74cf8d 2021-10-22 14:55:07 +00:00 Compare
lx force-pushed netapp from 6d8b74cf8d to 88a91fe648 2021-10-22 14:56:05 +00:00 Compare
lx force-pushed netapp from e84432181f to 060ed9dcd0 2021-10-22 19:56:37 +00:00 Compare
lx force-pushed netapp from 060ed9dcd0 to cda4523872 2021-10-22 20:04:19 +00:00 Compare
lx force-pushed netapp from eb8c93d50d to 6ab65b6ef4 2021-10-25 07:03:11 +00:00 Compare
lx force-pushed netapp from 6ab65b6ef4 to d5a8ce7fc7 2021-10-25 12:00:05 +00:00 Compare
lx force-pushed netapp from d5a8ce7fc7 to df8a4068d9 2021-10-25 12:21:56 +00:00 Compare
lx added 1 commit 2021-10-25 13:25:25 +00:00
Disable i686 and armv6l pipelines for now
All checks were successful
continuous-integration/drone/pr Build is passing
Details
continuous-integration/drone/tag Build is passing
Details
continuous-integration/drone Build is passing
Details
continuous-integration/drone/push Build is passing
Details
f6ebcbc7a7
lx merged commit f6ebcbc7a7 into main 2021-10-25 13:49:35 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
quentin
Labels
Clear labels   
action
check-aws
The behaviour should be checked against AWS S3

  
action
discussion-needed
We need to discuss if the feature is desirable and/or better specify the envisioned behavior or technical implementation

  
action
for-external-contributors
Core team does not plan working on this in the near future however an external contribution will be reviewed & merged

  
action
for-newcomers
An issue that should be newcomer friendly.

  
action
more-info-needed
More information is needed to investigate this issue

  
action
need-funding
This issue represents lots of work, we need to find money to pay our core team work

  
action
triage-required
It's not very clear what we want to do with this issue yet, more thoughts is required

  
kind
correctness
Issues of theoretical correctness that may impact edge-case scenarios

  
kind
ideas
For abstract ideas/proposal to make Garage better, to better communicate our not yet achieved aims

  
kind
improvement
Feature requests that do not fall in other categories

  
kind
performance
Deceptive performance observed OR idea to improve performances

  
kind
testing
Functional tests, integration tests, Jepsen, etc.

  
kind
usability
Propositions to make Garage easier to operate, by improving error reporting, CLI, etc.

  
kind
wrong-behavior
Unintended behavior

  
prio
critical
Could impact integrity or availability of production clusters

  
prio
low
For issues that we don't want to spend time on until made absolutely necessary

  
scope
admin-api
Issues concerning the administration API

  
scope
background-healing
Background healing covers repair tasks, anti-entropy, merkle trees, etc. ; everything related to replicating data appropriately.

  
scope
build
Issues with Nix, Drone, Rust Compiler, etc.

  
scope
documentation

  
scope
k8s
Kubernetes related, including Helm Charts

  
scope
layout
Linked to the cluster layout and storage management

  
scope
metadata
Metadata Engine (LMDB, sqlite, etc.)

  
scope
ops
Operations related to operations/maintenance issue, tooling related (like CLI missing info ; impossible, too dangerous, too complicated migration, etc.)

  
scope
rpc
Linked to RPC

  
scope
s3-api
Related to the S3 API (especially compatibility issues).

  
scope
security
Cryptography-related, auth-related, authorization-related issues

  
scope
telemetry
OpenTelemetry (traces, metrics, etc.)

No labels
action
check-aws
action
discussion-needed
action
for-external-contributors
action
for-newcomers
action
more-info-needed
action
need-funding
action
triage-required
kind
correctness
kind
ideas
kind
improvement
kind
performance
kind
testing
kind
usability
kind
wrong-behavior
prio
critical
prio
low
scope
admin-api
scope
background-healing
scope
build
scope
documentation
scope
k8s
scope
layout
scope
metadata
scope
ops
scope
rpc
scope
s3-api
scope
security
scope
telemetry
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/garage#123
No description provided.
Delete branch "netapp"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?