-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Avoid build on linera net up --kubernetes #2986
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ impl DockerImage { | |
| &self.name | ||
| } | ||
|
|
||
| pub async fn build(name: String, binaries: &BuildArg, github_root: &PathBuf) -> Result<Self> { | ||
| pub async fn build(name: &String, binaries: &BuildArg, github_root: &PathBuf) -> Result<Self> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| let build_arg = match binaries { | ||
| BuildArg::Directory(bin_path) => { | ||
| // Get the binaries from the specified path | ||
|
|
@@ -73,12 +73,7 @@ impl DockerImage { | |
| ), | ||
| ]); | ||
|
|
||
| command | ||
| .arg(".") | ||
| .args(["-t", &name]) | ||
| .spawn_and_wait() | ||
| .await?; | ||
|
|
||
| command.arg(".").args(["-t", name]).spawn_and_wait().await?; | ||
| Ok(docker_image) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,8 @@ pub struct LocalKubernetesNetConfig { | |
| pub num_initial_validators: usize, | ||
| pub num_shards: usize, | ||
| pub binaries: BuildArg, | ||
| pub no_build: bool, | ||
| pub docker_image_name: String, | ||
| pub policy: ResourceControlPolicy, | ||
| } | ||
|
|
||
|
|
@@ -62,6 +64,8 @@ pub struct LocalKubernetesNet { | |
| next_client_id: usize, | ||
| tmp_dir: Arc<TempDir>, | ||
| binaries: BuildArg, | ||
| no_build: bool, | ||
| docker_image_name: String, | ||
| kubectl_instance: Arc<Mutex<KubectlInstance>>, | ||
| kind_clusters: Vec<KindCluster>, | ||
| num_initial_validators: usize, | ||
|
|
@@ -96,6 +100,8 @@ impl SharedLocalKubernetesNetTestingConfig { | |
| num_initial_validators: 4, | ||
| num_shards: 4, | ||
| binaries, | ||
| no_build: false, | ||
| docker_image_name: String::from("linera:latest"), | ||
| policy: ResourceControlPolicy::devnet(), | ||
| }) | ||
| } | ||
|
|
@@ -122,6 +128,8 @@ impl LineraNetConfig for LocalKubernetesNetConfig { | |
| self.network, | ||
| self.testing_prng_seed, | ||
| self.binaries, | ||
| self.no_build, | ||
| self.docker_image_name, | ||
| KubectlInstance::new(Vec::new()), | ||
| clusters, | ||
| self.num_initial_validators, | ||
|
|
@@ -300,10 +308,13 @@ impl LineraNet for LocalKubernetesNet { | |
| } | ||
|
|
||
| impl LocalKubernetesNet { | ||
| #[allow(clippy::too_many_arguments)] | ||
| fn new( | ||
| network: Network, | ||
| testing_prng_seed: Option<u64>, | ||
| binaries: BuildArg, | ||
| no_build: bool, | ||
| docker_image_name: String, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is All the futures are joined at the end of the function so there are no borrows which will outlive the reference (I think). |
||
| kubectl_instance: KubectlInstance, | ||
| kind_clusters: Vec<KindCluster>, | ||
| num_initial_validators: usize, | ||
|
|
@@ -315,6 +326,8 @@ impl LocalKubernetesNet { | |
| next_client_id: 0, | ||
| tmp_dir: Arc::new(tempdir()?), | ||
| binaries, | ||
| no_build, | ||
| docker_image_name, | ||
| kubectl_instance: Arc::new(Mutex::new(kubectl_instance)), | ||
| kind_clusters, | ||
| num_initial_validators, | ||
|
|
@@ -394,8 +407,12 @@ impl LocalKubernetesNet { | |
| async fn run(&mut self) -> Result<()> { | ||
| let github_root = get_github_root().await?; | ||
| // Build Docker image | ||
| let docker_image = | ||
| DockerImage::build(String::from("linera:latest"), &self.binaries, &github_root).await?; | ||
| let docker_image_name = if self.no_build { | ||
| self.docker_image_name.clone() | ||
| } else { | ||
| DockerImage::build(&self.docker_image_name, &self.binaries, &github_root).await?; | ||
| self.docker_image_name.clone() | ||
| }; | ||
|
|
||
| let base_dir = github_root | ||
| .join("kubernetes") | ||
|
|
@@ -412,13 +429,13 @@ impl LocalKubernetesNet { | |
|
|
||
| let mut validators_initialization_futures = Vec::new(); | ||
| for (i, kind_cluster) in self.kind_clusters.iter().cloned().enumerate() { | ||
| let docker_image_name = docker_image.name().to_string(); | ||
| let base_dir = base_dir.clone(); | ||
| let github_root = github_root.clone(); | ||
|
|
||
| let kubectl_instance = kubectl_instance_clone.clone(); | ||
| let tmp_dir_path = tmp_dir_path_clone.clone(); | ||
|
|
||
| let docker_image_name = docker_image_name.clone(); | ||
| let future = async move { | ||
| let cluster_id = kind_cluster.id(); | ||
| kind_cluster.load_docker_image(&docker_image_name).await?; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the link with the kubernetes feature and why
--kubernetesin the title of this PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--kubernetescalls the kubernetes version oflinera net up, which needs to be compiled with thekubernetesfeature. But if you don't compile with thekubernetesfeature, the--kubernetesoption is not even available.You can still compile with the
kubernetesfeature and run justlinera net upwithout--kubernetes, and it won't run the kubernetes versionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I misread the test plan (which could have mentioned these extra arguments)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It's hard to understand the context because the motivation section doesn't mention kubernetes (and locally superficially seems contradictory although now I remember we have
kind).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I edited the motivation, hopefully it's clearer now