sometimes deploying is not working - race conditions with health_checks ?

###########################################################################################################################################
I, [2015-07-03T15:56:47.655965 #29464]  INFO -- : Checking if any deployments are already running for application some-webapp
I, [2015-07-03T15:56:47.726509 #29464]  INFO -- : Starting deployment of some-webapp
/var/lib/gems/1.9.1/gems/marathon_deploy-0.1.3/lib/marathon_deploy/deployment.rb:157:in `health_checks_defined?': undefined method `[]' for nil:NilClass (NoMethodError)
	from /var/lib/gems/1.9.1/gems/marathon_deploy-0.1.3/lib/marathon_deploy/marathon_client.rb:69:in `deploy'
	from /var/lib/gems/1.9.1/gems/marathon_deploy-0.1.3/bin/marathon_deploy:130:in `block in <top (required)>'
	from /var/lib/gems/1.9.1/gems/marathon_deploy-0.1.3/bin/marathon_deploy:124:in `each'
	from /var/lib/gems/1.9.1/gems/marathon_deploy-0.1.3/bin/marathon_deploy:124:in `<top (required)>'
	from /usr/local/bin/marathon_deploy:23:in `load'
	from /usr/local/bin/marathon_deploy:23:in `<main>'
Build step 'Execute shell' marked build as failure
Finished: FAILURE

fixed.


bug: KS v0.21 latency for remote pod execution degraded relative to v0.20 for Braulio stress test

Describe the bug
Braulio's stress test creates pods in a WDS at a specified rate. The pods run "bash -c sleep 20" on the ubuntu:latest image and then terminate. In my test KS was deployed on a VM (32GB RAM, 8vCPU) using K3D clusters for hub and spoke.
The two plots below show individual pod end-to-end latencies from creation time in WDS to status=complete in WDS. Pods were created 1 every 3 seconds in the same namespace. A single binding policy was used. There were no pod deletions during each test.

Steps To Reproduce
A PR adding a document describing how to deploy KS on K3D should be available shortly.
Braulio will document how to run his stress test. Yaml files for the pod and and the binding policy are included here.
yamls.zip
Expected Behavior
KS overhead latency should not be so badly degraded for this test.
Additional Context
No response

@eaepstein @dumb0002 thanks for opening this issue, indeed an important one that worth deeper investigation.
would it be possible to add more information about where do we spend the time?
it would be very helpful to understand what exactly takes time to identify where is the problem.
some examples -

does the distribution from WDS to ITS take more time?
does OCM distribution from ITS to WECs take more time? in this case we might hit OCM bug (see issue #1868)
it could be that distribution works perfectly fine but the issue is in the status?
etc...

@nirrozenbaum I don't think it's an issue of "where we spend the time" , the main problem I see is a (linear) constant increase in time as the number of resources  increases.  Which means this is a major scalability issue.

@eaepstein @nirrozenbaum One test that can be helpful is:

Measure latency of creation
Populate the system with large number of objects
Wait for few minutes
Repeat (1) - Measure latency of creation
If 4 is >> from 1 we have  a scaling issue that can be easily profiled and debugged separately (out of the "stress test" environment)

@ezrasilvera before you declare scalability issue, I would first try to analyze things in a more fine-grained granularity.
we may find a bug that's causing this and upon solving the bug we may find out things are working as expected.
we may also find out the issue is related to OCM and not to KS.
therefore, it's important to understand where we spend the time and what takes so long.
it should be easy to get this information.

This is a very interesting issue.  It would be interesting to see the time spent in each phase (object creation in WDS, binding object creation, manifestwork object creation, object creation in WEC).  I am hoping this can be easily obtained from the creationTimestamp field in the different objects.

@nirrozenbaum I'm not sure I follow your point - The test I was proposing is the first step to such analysis .   If you see consistent increase in  the test I proposed you can easily profile it and see exactly where the times were spent.
If you don't see that increase , it means that it also related to the dynamic of the test (rate of creation, creation+status delivery, etc..) and this is much harder to profile.

Also - a simple test would be to repeat the exact same test without the "Want Status"  .   While continue getting similar results won't prove that's it's still not related to the status, getting improved results can prove that the latency is related to status.

@ezrasilvera ok, you're saying what I was saying in different words.
I was asking to see where do we spend time, while you are suggesting ways to check it.
so we're basically requesting for the same using different words :).

@dumb0002 (cc: @eaepstein )  Can you also attach or point to the script you use to test and to measure the timing (in case we need to reproduce)

@nirrozenbaum @ezrasilvera @effi-ofer (cc; @eaepstein) - sure, I will investigate where we spend the time in each phase to identify any bottleneck phase in the overall e2e latency illustrated in the graph above.  @ezrasilvera, here a draft README.md on how to run the scripts: https://github.ibm.com/MCC/MCC/tree/mcc-exp-v2/exp

@dumb0002 I think it's good that KS would have a performance section. I understand that currently your tests are in the MCC scope but maybe they can be easily updated to run directly on top of KS.
I created https://github.com/kubestellar/kubestellar/issues/1960   to track that.
We also need to find a good place to store the results under the KS git

@nirrozenbaum Braulio's data collection was designed for separate manifestwork objects for each workload object, so the standard data analysis is broken for wds-pod-create -> imbs-manifestwork-updated. Does each entry in the bundled manifestwork have a create time? Do entries also have the ID of the associated workload objects to be able to measure transport times?
The plot below measures from wds-pod-create -> wec-pod-create, so it includes OCM time. But it also makes it clear that the end-to-end latency degradation is due to the effect of bundling, as nothing else changed.

@ezrasilvera, my current stress test can run directly on top of KS without changes, I'll improve the readme and move it to the KS repo (using  #1960 to keep track of it)

thanks @eaepstein and @dumb0002.
if we keep creating pods at the same rate, do we get to a stable graph after a while? or is it continuously keep growing?
is it possible to run this test for 30 minutes (or even more) and show the output?

@nirrozenbaum The longer run of ks0.21 stopped down-syncing at 187 pods. Latencies were continuing to go up.
There was nothing notable in the klusterlet or transport-ctlr logs. At some point as the existing pods were being deleted from WDS, the remaining pending pods began downsyncing again.
A ks0.20 test of 400 pods completed. The latency charts are below.

thanks @eaepstein and @dumb0002. if we keep creating pods at the same rate, do we get to a stable graph after a while? or is it continuously keep growing? is it possible to run this test for 30 minutes (or even more) and show the output?

@nirrozenbaum While it is interesting question, I'm not sure why we need to discuss it or try it now.  I think what we already see  is good enough reason to profile and explore the reason for the linear growth in latency.  The only question is who is going to do it..   I'm not sure that the person who is going to profile and analyze it needs more info from @dumb0002 ..
We simply need someone to take this issue and investigate.

@ezrasilvera what I was trying to understand is if it reaches some stability. if yes, there is no scale issue, cause we may see the scale continuously growing (the pod creation rate is faster than completion rate) and there is no increase in time.
I think this is a very important input.
it could be that in this specific setup (which I was continuously describing as not representative for KS), in the warmup phase of the test the measured time is slowly growing but then reaches stability.
if that would be the case I would be very happy to see that. that's a very good sign.
of course we can assign a person to further investigate but there's a major difference whether the issue is in the warmup or a general issue and it keeps growing and growing.

@nirrozenbaum The longer run of ks0.21 stopped down-syncing at 187 pods. Latencies were continuing to go up. There was nothing notable in the klusterlet or transport-ctlr logs. At some point as the existing pods were being deleted from WDS, the remaining pending pods began downsyncing again.
A ks0.20 test of 400 pods completed. The latency charts are below.

@eaepstein thanks for the input. I just noticed this input AFTER answering Ezra.
@ezrasilvera yes.  it's definitely worth investigating and see where exactly is the problem.

sharing an initial analysis -
in ks0.20 each object was wrapped individually. therefore, even though all pods are mapped to the WEC using a single BindingPolicy, the result of a new pod every 3 seconds is a new ManifestWork that includes one pod every 3 seconds.
that makes the creation rate + the size of ManifestWork stable. we can clearly see in the graph that 0.20 seems to at stable rate + size right from the beginning (the average seems to be somewhere around 24 sec per pod).
in ks0.21 the appraoch we took is different.
ManifestWork includes all objects that were mapped to a WEC using a single BindingPolicy. since this test is using a single BindingPolicy to map all pods, that means that in each iteration of the test, the ManifestWork object keep growing and growing. from an initial analysis it makes sense that the time to sync it to WEC keeps growing (cause the object is larger in every iteration). then at some point it seems not to sync anymore. intuitively I think it could be that the ManifestWork object exceeded the limit of a k8s resource size (the limit is 1MB per resource, this is a general limit of k8s not related to ManifestWork) since it tries to put more than 187 pods in a single ManifestWork.
at some point when pods are starting to go away, the pending pods (all the ones above the 187) are starting to sync again. this assumption has to be further investigated and if correct, we already talked about it in the past to introduce a mechanism for fragmentation (splitting into more than one manifestwork).
that's an advanced topic so it was not included in the first transport version.
according to this analysis, the graph makes sense to me.
the question then goes back to the original question I asked -
is this a representative stress test for KS? I think we agreed it's not.
it's not expected from a KS user to use a single BindingPolicy to distribute all objects that ever exist, although that's not prohibited.
I think we need stress the system in a more representative test and look if there are major differences in performance.
If that's ok with all of you, give me around two weeks and I'll try to come up with stress test setup that represents KS usage better. we can then discuss it and continue investigating.
/cc @vMaroon

@nirrozenbaum A single binding policy could be in place for multiple users on one WDS, each in their own namespace. The degradation is pretty bad even with low numbers, and appwrapper objects could be much bigger than pods.
It would interesting to find a realistic scenario for which bundling per binding policy has significantly better performance in some metric: latency, memory, or CPU.

@nirrozenbaum, for the use-case of MCAD +  ks0.20 integration, a decision was made to use a single BindingPolicy per remote clusters created at the bootstrap of the env: that user does not directly interacts with KS and uses upper layer application to select where to deploy its workload - This stress test experiment was representing such use-case.
For ks0.21, it would be interesting to evaluate the performance for both extreme cases: 1) a binding policy per objects  and 2) a single binding policy for all the objects (the current test). In addition to evaluate some cases in between.
Which more representative use-case for test do you have in mind? Let's have more discussion in this - we can configure the tool that we are using to generate the workload to represent such a scenario - clusterloader2 can be easily configured.

@nirrozenbaum A single binding policy could be in place for multiple users on one WDS, each in their own namespace. The degradation is pretty bad even with low numbers, and appwrapper objects could be much bigger than pods. It would interesting to find a realistic scenario for which bundling per binding policy has significantly better performance in some metric: latency, memory, or CPU.

@eaepstein I didn't understand this comment. `BindingPolicy is a cluster-scoped resource. what do you mean when you say single binding policy for multiple users on one WDS, each in their own namespace?
BindingPolicy isn't in a namespace.. can you please explain?

@nirrozenbaum, for the use-case of MCAD + ks0.20 integration, a decision was made to use a single BindingPolicy per remote clusters created at the bootstrap of the env: that user does not directly interacts with KS and uses upper layer application to select where to deploy its workload - This stress test experiment was representing such use-case.
For ks0.21, it would be interesting to evaluate the performance for both extreme cases: 1) a binding policy per objects and 2) a single binding policy for all the objects (the current test). In addition to evaluate some cases in between.
Which more representative use-case for test do you have in mind? Let's have more discussion in this - we can configure the tool that we are using to generate the workload to represent such a scenario - clusterloader2 can be easily configured.

regarding representative test for KS - I think we discussed it several times with the team. if a project has to manage 2 clusters and for each of them they distribute one wrapped object, they probably wouldn't use KS for that :). KS has a big overhead if the purpose is managing only 2 clusters and for each deploying one object.
if we put aside MCAD integration (although we definitely need to support MCAD use case), I would say a representative use case for KS would be something along these lines -
having 10-100 WECs.
having 10 different users, some of them could share WDS, so let's say we spread them over 3-4 WDS.
each user creates 10 BindingPolicy objects, each selects 5-10 WECs, in each they have 20 objects.
I would expect that a workload that users distributes with a single binding policy to have some relation between the objects. for example - a workload (set of objects) may be depoyment, secret, serviceaccount, role, rolebinding, configmap, etc.. with some dependencies between them. e.g., the deployment uses the service account, the secret and configmap and service account gets its permissions using the role+rolebinding.
again, we do not prohibit distributing in one policy arbitrary set of objects with no relation between them, but that doesn't really makes sense from a user point of view. usually k8s users use labels on objects to map them to some application that runs on the cluster.
so I think we should aim to test the system a setup closer to the above rather than the extreme cases. extreme cases are not the representative cases.

@nirrozenbaum, thanks for the detailed explanation above. I will start looking at open-source tools to simulate k8s clusters as we'll need simulation to experiment with large number of  WECs.  I still think that to fully understand the performance of system it is important to test for representative cases, as well as the extreme cases - it is important to understand how a system behaves when it is pushed to its limit but we don't have to necessarily fix it for the extreme cases.

@nirrozenbaum, thanks for the detailed explanation above. I will start looking at open-source tools to simulate k8s clusters as we'll need simulation to experiment with large number of WECs. I still think that to fully understand the performance of system it is important to test for representative cases, as well as the extreme cases - it is important to understand how a system behaves when it is pushed to its limit but we don't have to necessarily fix it for the extreme cases.

right. I agree.
the performance optimizations should be oriented to the representative cases.
so after seeing a comparison with those cases we can optimize one way or another (depends on results and where are bottlenecks). just to make sure it's clear - if in a representative cases we'll see that wrapping individual objects performs better I will change the implementation to do that. I'm just not so sure that would be the case when we start distributing and updating objects in parallel which simulates real system with multiple users.
the existing test lacks parallelism, not multiple objects (manifestwork), no multiple updates in parallel (cause all pods are mapped to a single manifestwork so updates are serial), so this test is completely serial instead of parallel.
having said the above, extreme cases SHOULD WORK and not stop distributing (this is where fragmentation will come in).
so I think the ideal is - optimization should be oriented to representative case, but extreme cases should also work even if not with optimized performance.
let's keep syncing on this topic to identify bottlenecks and decide on the required optimizations.

@nirrozenbaum (cc: @dumb0002 @eaepstein )  Without getting into the two architecture options and the pros/cons, I think there are some alarming points here , some might even be considered as blockers that MUST be fixed (there are several different ways to fix them).

The latency degradation itself is very problematic, while we know the reason it is still considered as unacceptable behavior in terms of user expectation.  Users expect to get the same latency/response behavior regardless of the system load  or scale, this is actually the definition of the scalability challenge.

Note, that putting each object in a different manifest may have similar impact on other scenarios

KS should never stop syncing. We should publish official limits (number of object, objects per manifest, etc.. ) and implement an active limit function that block+warn the user. We can then review these limits and decide whether they are reasonable for most of our use cases or not.
BTW, if @dumb0002 would have used larger objects the linear slope would have been even larger and the syncing would have stopped much sooner.

@nirrozenbaum I think it's time to pause and prepare the comparison table between the options and list all these scenarios. As your analysis shows the latency increase wasn't really a surprise and we could have anticipated  such behavior  and put it in the table before @dumb0002 even tested it (we could then conducted tests just to measure the actual numbers) .

@nirrozenbaum A single binding policy could be in place for multiple users on one WDS, each in their own namespace. The degradation is pretty bad even with low numbers, and appwrapper objects could be much bigger than pods. It would interesting to find a realistic scenario for which bundling per binding policy has significantly better performance in some metric: latency, memory, or CPU.

@eaepstein I didn't understand this comment. `BindingPolicy is a cluster-scoped resource. what do you mean when you say single binding policy for multiple users on one WDS, each in their own namespace? BindingPolicy isn't in a namespace.. can you please explain?

Right, the single binding policy will apply to all namespaces in the WDS. So assuming all of the user created objects have policy matching labels, they would be placed in the same bundle.

@nirrozenbaum (cc: @dumb0002 @eaepstein ) Without getting into the two architecture options and the pros/cons, I think there are some alarming points here , some might even be considered as blockers that MUST be fixed (there are several different ways to fix them).

The latency degradation itself is very problematic, while we know the reason it is still considered as unacceptable behavior in terms of user expectation.  Users expect to get the same latency/response behavior regardless of the system load  or scale, this is actually the definition of the scalability challenge.

Note, that putting each object in a different manifest may have similar impact on other scenarios

KS should never stop syncing. We should publish official limits (number of object, objects per manifest, etc.. ) and implement an active limit function that block+warn the user. We can then review these limits and decide whether they are reasonable for most of our use cases or not.
BTW, if @dumb0002 would have used larger objects the linear slope would have been even larger and the syncing would have stopped much sooner.

@nirrozenbaum I think it's time to pause and prepare the comparison table between the options and list all these scenarios. As your analysis shows the latency increase wasn't really a surprise and we could have anticipated such behavior and put it in the table before @dumb0002 even tested it (we could then conducted tests just to measure the actual numbers) .

@ezrasilvera I completely disagree on point (1). no user expects to get the same latency if they distribute manifestwork with one object or manifestwork with 20 objects. the size of the object is (almost) 20 times bigger and it's obvious that it takes more time to sync it.

@nirrozenbaum A single binding policy could be in place for multiple users on one WDS, each in their own namespace. The degradation is pretty bad even with low numbers, and appwrapper objects could be much bigger than pods. It would interesting to find a realistic scenario for which bundling per binding policy has significantly better performance in some metric: latency, memory, or CPU.

@eaepstein I didn't understand this comment. `BindingPolicy is a cluster-scoped resource. what do you mean when you say single binding policy for multiple users on one WDS, each in their own namespace? BindingPolicy isn't in a namespace.. can you please explain?

Right, the single binding policy will apply to all namespaces in the WDS. So assuming all of the user created objects have policy matching labels, they would be placed in the same bundle.

you assume that all objects created by a user have the same label? and that label is the one that is used in policy to select the objects? or user is using multiple label selectors with the purpose of selecting all his created objects?
if that's the case - then of course, all will be mapped to the same bundle..
if that user has a single application he wants to run that might include 10-20 objects it might even make sense.
but in other cases, why would a user do that?
why would a user put the same label on all objects or select all his objects if that's not related to the same application?
again, this is not prohibited in k8s. but I think there is a misunderstanding of what's the purpose of labels in k8s.
users of k8s do not label all objects with the same label because that's missing the whole point.
the most common use case for using k8s labels is for **Grouping and Organizing -
Labels can be used to group related resources together. For example, you can label all backend services with app=backend and all frontend services with app=frontend.
there are additional use cases for labels like:
Identification and Selection: Labels are used by selectors to identify and select resources. For instance, a Deployment may use a selector to specify which Pods it should manage based on their labels.
Service Discovery: Labels are often used in conjunction with Services for service discovery. Services can route traffic to Pods based on labels and selectors.
Configuration and Policies: Labels can be used to apply configurations and policies to specific sets of resources. For example, you can apply resource quotas or network policies based on labels.
none of the use cases includes labeling all objects with the same label (unless it's all related as part of a single application, which is usually not a lot of objects).. that doesn't really make sense.

@ezrasilvera I completely disagree on point (1). no user expects to get the same latency if they distribute manifestwork with one object or manifestwork with 20 objects. the size of the object is (almost) 20 times bigger and it's obvious that it takes more time to sync it.

That's not the case at hand.  The user didn't create 20 Pods and tried to sync them. In that case your statement is correct . However, I thought the user was coming and was just deploying the 20th Pod as a single Pod , while using the existing ManifestWork.
In that case the user doesn't know the internal implementation details. He just observed that few our ago when he deployed a Pod it took X sec and now when he is deploying a Pod (which may be the 100th Pod)  it takes much longer.

do you assume that deploying a new pod will be mapped to the same binding policy as other pods? why?
again, anyone can come up with many unreasonable use cases.
do you any k8s user trying to map all pods to be managed by a single deployment (e.g., deployment managing 400-500 pods)?
OR
do you see different deployment objects where each is managing several pods (for example 10) and each deployment is managing a DIFFERENT set of pods?
we all know the answer :)
that's exactly the same with BindingPolicy, it makes no sense to map a single policy to 400 pods or more.
I don't know why we keep raising/analyzing use cases that make no sense.
have you ever tried to create a single deployment in k8s to manage few hundreds of pods in a cluster?

@dumb0002 may I ask you to run a very similar test with simple change and measure the times? (if possible with graph as in your previous test).
can you try to run the same test BUT change the creation order?
for example -
first, create 100 pods all with the same label. even though I don't think it sounds reasonable to do that, but just to compare apples to apples.
then, create the BindingPolicy to distribute all pods to a WEC.
let's measure the time it takes to distribute 100 pods in the two options (ks 0.20 and ks0.21) from the moment we create the BindingPolicy until all 100 pods have completed.
just to set the expectations in terms of analysis -
in ks0.20 - a burst of 100 MW objects will be created (Klusterlet will need to sync 100 MW to WEC).
in ks0.21 - one MW will be created.

btw, I’d be happy to hear your opinions, both options are possible but what do you think makes more sense -

create workload objects with some labels and then create BindingPolicy to select those objects.
OR
create BindingPolicy that selects nothing using labels that doesn’t exist (so selection is empty) and then start adding workload objects with the requested labels and the selection is updated in each addition.

Rather than have manifestwork contain either 1 object or all objects, another option is to introduce a limit on the number of embedded objects in manifestwork.  We can even introduce an optimization where on initial creation the manifestwork contains all matching objects even if greater than the limit.

having said the above, extreme cases SHOULD WORK and not stop distributing (this is where fragmentation will come in).

@effi-ofer exactly. this is what I was mentioning in one of the previous messages.
fragmentation is splitting MW when it reaches a limit (not exceeding a pre-defined size limit).
that's not implemented yet. but it should be added.

@dumb0002 may I ask you to run a very similar test with simple change and measure the times? (if possible with graph as in your previous test). can you try to run the same test BUT change the creation order? for example - first, create 100 pods all with the same label. even though I don't think it sounds reasonable to do that, but just to compare apples to apples. then, create the BindingPolicy to distribute all pods to a WEC. let's measure the time it takes to distribute 100 pods in the two options (ks 0.20 and ks0.21) from the moment we create the BindingPolicy until all 100 pods have completed.
just to set the expectations in terms of analysis - in ks0.20 - a burst of 100 MW objects will be created (Klusterlet will need to sync 100 MW to WEC). in ks0.21 - one MW will be created.

@nirrozenbaum, sure! I will conduct this experiment today

btw, I’d be happy to hear your opinions, both options are possible but what do you think makes more sense -

create workload objects with some labels and then create BindingPolicy to select those objects.
OR
create BindingPolicy that selects nothing using labels that doesn’t exist (so selection is empty) and then start adding workload objects with the requested labels and the selection is updated in each addition.

@nirrozenbaum, option 1 makes more sense to me to measure the time will take to distribute a burst of 100 pods all with the same label.

I don’t agree with most of your claims.
there is a best practice for working with k8s and more specifically in multi cluster management. there are some leading projects in this area and there is not even one that uses the pattern you’re suggesting. all of them are wrapping multiple objects that have a relation between them (in KS this is the workload). same definition exists in SIG multi cluster.
the pattern you suggested is indeed naive.
I didn’t say we need to tell users to create workloads first and then binding policy. what I do say is that there is a big difference in performance that depends on the creation order.
if we first create Bindingpolicy and then 100 pods we will see the attached graph above.
on the other hand, if we first create 100 pods and then the bindingpolicy we will see a different graph (I’m waiting patiently until @dumb0002 will upload it).
so now the question- which of these use cases should we optimize for?

@eaepstein btw, the naive suggestion completely ignores basic aspects of k8s, for example RBAC.
in a typical k8s cluster, user A has access to namespace nsA and user B has access to nsB. each user creates his objects in his dedicated ns. the naive approach suggesting one BindingPolicy for WEC, so every object that is mapped to that WEC is selected by the same BindingPolicy.
so now the questions - who creates this BindingPolicy? who is the owner of the object? who can remove it? user A? user B? user admin?
and a security question from a user point of view, if you're user A - do you want to share a BindingPolicy with user B?
in short, the answer is no. from the same reason they don't share a namespace on the hub. users that don't share permissions on the hub should NOT share BindingPolicy. there are many security issues here.
there are more k8s basic aspects that this suggestion is ignoring but I'll stop here.
there are some wrong assumptions along the way that are made due to the fact that we as developers work with an admin account. this is not the way users work in k8s eco system. users in any k8s system work with RBAC. we've not implemented (yet) any RBAC mechanism in KS, but that doesn't mean we can ignore it.

@nirrozenbaum  (cc: @eaepstein ), here are the plots for time it takes to distribute a burst of 100 pods from the moment we create the BindingPolicy until all 100 pods have completed. I used the following equations to compute the latency:
 [t_pod-wec-completion - t_bindingPolicy-creation] - alpha

where alpha = t_pod-wec-completion - t_pod-wec-creation  (this parameter removes the latency of the pod execution time in the WEC)

For burst workload the pod distribution is better for ks-0.21

@eaepstein btw, the naive suggestion completely ignores basic aspects of k8s, for example RBAC. in a typical k8s cluster, user A has access to namespace nsA and user B has access to nsB. each user creates his objects in his dedicated ns. the naive approach suggesting one BindingPolicy for WEC, so every object that is mapped to that WEC is selected by the same BindingPolicy. so now the questions - who creates this BindingPolicy? who is the owner of the object? who can remove it? user A? user B? user admin? and a security question from a user point of view, if you're user A - do you want to share a BindingPolicy with user B? in short, the answer is no. from the same reason they don't share a namespace on the hub. users that don't share permissions on the hub should NOT share BindingPolicy. there are many security issues here.

Aren't binding policies cluster scoped? If so, with an RBAC that blocks normal users from creating cluster scoped objects neither user A nor user B would be allowed to create them.

@nirrozenbaum (cc: @eaepstein ), here are the plots for time it takes to distribute a burst of 100 pods from the moment we create the BindingPolicy until all 100 pods have completed. I used the following equations to compute the latency:
 L = [t_pod-wec-completion - t_bindingPolicy-creation] - alpha

where alpha = t_pod-wec-completion - t_pod-wec-creation  (this parameter removes the latency of the pod execution time in the WEC)

For burst workload the pod distribution is better for ks-0.21

@dumb0002 many thanks for conducting this experiment.
so what we see here is the following -
when creating first the workload objects and then the BindingPolicy in this experiment, the time to completion using ks0.21 was 32 seconds vs 2:10 minutes us ks0.20.
on the other hand when creating first the binding policy and then 100 workload objects, the 100th pod completion time take 23-24 second in ks0.20 vs 1:09 in ks0.21.
this experiment is nice and shows differences in performance depends on creation order.
clarification - I don't think we should recommend any creation order, users should create in any order they want.
even though this experiment is nice, this is a serial test (everything is done serially), so this is very far away from reality in most cases.
to have a good test for KS, we should design and conduct experiments that use parallelism with some randomness.
e.g., in some cases add/update/delete objects in existing BindingPolicy (which in turn updates existing MW), in other cases create new objects and distribute them using new BP, in some cases create objects first then BP and in others switch the creation order. all of this should happen in parallel by multiple threads that will simulate multiple users.
and of course, we need to define what are the metrics we want to compare.

Aren't binding policies cluster scoped? If so, with an RBAC that blocks normal users from creating cluster scoped objects neither user A nor user B would be allowed to create them.

@eaepstein if I it right, you suggest that KS users won't have access to BindingPolicy and it's a system auto-created object per WEC.
your suggestion ignores RBAC and more specifically users isolation, it ignores grouping and organization of WECs which is a basic aspect in large scale (it ignores scale in general), it ignores status summarization and status in general, it ignores the option for dynamic changes like dynamic customization and filtering, and more..
By design, BindingPolicy is a user facing API. that's the entry point for users to define things dynamically including how to summary status, how to dynamically customize and filter objects (see for example the latest requirement from Argo integration here - #1976).
I invite you to raise this suggestion for the broader team (slack/poll/webex.. doesn't matter) and see their responses.
I doubt this is the model we want to sell for KS.

I have been looking into several use cases for KubeStellar and integrations with other projects such as Argo Workflows, MCAD, Kueue and KubeFlow Pipelines. None of these use cases would require hundreds or thousands of clusters, but rather a handful of them. On the other hand, objects to deliver could be quite large (an Argo Workflow object may contain python code, plenty of metadata and lots of pod specs). In most of these integrations, the simplest approach is to create one policy per cluster (typically, each workflow or job would be delivered just to one cluster) and rely simply on labels or other identifiers in the objects to route them to the selected target cluster. I think that one of the key goals of KubeStellar is to reduce friction for adopting multi-cluster, and it seems to me that putting the burden on the user to create for example one binding policy for each workload is asking too much. This is something we saw also with the MCAD integration, which was drastically simplified just labeling the MCAD objects. My recommendation is to a) evaluate performances with N larger workload objects per binding policy (I can provide argo workflows sample generated by KFP) b) work on a design for ManifestWork sharding based on ManifestWork size, where  max size per shard is tunable so that the it can adapt the different use cases.

@pdettori I agree that there are many use cases that just want WorkLoad -->single WEC (and then the main problem is scheduling algorithm).  However I don't want us to be biased from the (great) recent  integration work and design KS as if that's the main use case.
Remember that we started with use cases aimed for Edge and maybe "branch offices"  in which the common patter is WL --> many WECs.
BTW, even on your integration scenarios, if we also want to deploy the base components through KS (not just the workload) like - monitoring agents, agents etc.. -  that deployment is 1:N and not 1:1

@ezrasilvera I was just presenting use cases that are of immediate applicability for us. But independently from 1:1 or 1:N, the problem of a too large manifest could present itself, therefore we should be able to accomodate these different use cases.

any thanks for conducting this experiment. so what we see here is the following - when creating first the workload objects and then the BindingPolicy in this experiment, the time to completion using ks0.21 was 32 seconds vs 2:10 minutes using ks0.20. on the other hand when creating first the binding policy and then 100 workload objects, the 100th pod completion time take 23-24 second in ks0.20 vs 1:09 in ks0.21. this experiment is nice and shows differences in performance depends on creation order. clarification - I don't think we should recommend any creation order, users should create in any order they want.
even though this experiment is nice, this is a serial test (everything is done serially), so this is very far away from reality in most cases.

@nirrozenbaum, the plots posted at the beginning of this issue is the E2E latency using a sequential function to generate the workload in a K3D cluster. Hence, for accurate comparison, I repeated the experiment for creating first the binding policy and then a burst of 100 workload objects (there is parallelism in this burst workload generation):
To compute latency to distributed the pods to the WEC are used the following equation:
L = [t_pod-wec-completion - t_pod-wds1-creation] - alpha

where alpha = t_pod-wec-completion - t_pod-wec-creation  (this parameter removes the latency of the pod execution time in the WEC)

Comparing these plots we can observe that  ks0.21 performs better for burst workloads and Yes there is differences in performance depends on creation order of BindingPolicy & workload:
a)  when creating first the workload objects and then the BindingPolicy in this experiment, the time to completion using ks0.21 was 33 seconds vs 2:10 minutes using ks0.20
b)  when creating first the binding policy and then 100 workload objects, the time to completion using ks0.21 was 32 seconds vs 1:20 minutes using ks0.20.
I'll design some experiments with some degree of parallelism and randomness to discuss with the team so that we can all come to an agreement of a good representative workload and traffic patterns.

I have been looking into several use cases for KubeStellar and integrations with other projects such as Argo Workflows, MCAD, Kueue and KubeFlow Pipelines. None of these use cases would require hundreds or thousands of clusters, but rather a handful of them. On the other hand, objects to deliver could be quite large (an Argo Workflow object may contain python code, plenty of metadata and lots of pod specs). In most of these integrations, the simplest approach is to create one policy per cluster (typically, each workflow or job would be delivered just to one cluster) and rely simply on labels or other identifiers in the objects to route them to the selected target cluster. I think that one of the key goals of KubeStellar is to reduce friction for adopting multi-cluster, and it seems to me that putting the burden on the user to create for example one binding policy for each workload is asking too much. This is something we saw also with the MCAD integration, which was drastically simplified just labeling the MCAD objects. My recommendation is to a) evaluate performances with N larger workload objects per binding policy (I can provide argo workflows sample generated by KFP) b) work on a design for ManifestWork sharding based on ManifestWork size, where max size per shard is tunable so that the it can adapt the different use cases.

@pdettori, agreed, for KubeStellar and integrations with other upper level application, the user shouldn't be burden to create binding policies, instead it can rely simply on labels or other identifiers to route objects to the target remote cluster. That's what we're doing right now in our MCC stack.
Could you provide the sample  argo workflows generated by KFP that you mentioned above?

while things may make sense in one integration or another, the question here is deeper.
the question is about KS model and the design.
for example, if KS model for distribution is 1:1 that’s completely different than 1:N, and the design, architecture and optimizations will be made very differently depends on the model.

while things may make sense in one integration or another, the question here is deeper.
the question is about KS model and the design. for example, if KS model for distribution is 1:1 that’s completely different than 1:N, and the design, architecture and optimizations will be made very differently depends on the model.

Right. The one architectural design that has clear impact on the usage scenarios discussed above is that BPs are currently cluster-scoped. I also agree with this design choice because it offers simpler usage scenarios and better scalability.

To compute latency to distributed the pods to the WEC are used the following equation:
L = [t_pod-wec-completion - t_pod-wds1-creation] - alpha

where alpha = t_pod-wec-completion - t_pod-wec-creation  (this parameter removes the latency of the pod execution time in the WEC)

@dumb0002 These are surprising plots, particularly the grouping of latency values exactly on one second intervals for ks021. To disambiguate delays due to bundling+transport+klusterlet submission from delays due to K8S deploying the pods it could be interesting to see ks020 and ks021 plots of:
bundling+transport+klusterlet = t_pod-wec-creation - t_pod-wds1-creation

and
kubernetes deployment = t_pod-wec-completion - t_pod-wec-creation - 20

I notice that when running the stress test and metrics together, kubectl reports for wec1:
stress-pod-3865   0/1     Pending             0          0s
stress-pod-3865   0/1     Pending             0          0s
stress-pod-3865   0/1     ContainerCreating   0          0s
stress-pod-3865   1/1     Running             0          1s
stress-pod-3866   0/1     Pending             0          0s
stress-pod-3866   0/1     Pending             0          0s
stress-pod-3866   0/1     ContainerCreating   0          0s
stress-pod-3866   1/1     Running             0          1s

Delays from wec creating to running could be much longer in burst testing.

Right. The one architectural design that has clear impact on the usage scenarios discussed above is that BPs are currently cluster-scoped. I also agree with this design choice because it offers simpler usage scenarios and better scalability.

This wasn't what I was referring to. We all agree that BPs should be cluster-scoped. there is no other way to select cluster-scoped resources for distribution.
I was referring to the question of whether BPs are fixed, e.g., one per WEC, or dynamic, e.g., created dynamically per workload and includes grouping and organization of clusters, while each workload may include different set of clusters.
these were (some of) the design choices I was referring to

Right. The one architectural design that has clear impact on the usage scenarios discussed above is that BPs are currently cluster-scoped. I also agree with this design choice because it offers simpler usage scenarios and better scalability.

This wasn't what I was referring to. We all agree that BPs should be cluster-scoped. there is no other way to select cluster-scoped resources for distribution. I was referring to the question of whether BPs are fixed, e.g., one per WEC, or dynamic, e.g., created dynamically per workload and includes grouping and organization of clusters, while each workload may include different set of clusters. these were (some of) the design choices I was referring to

It could make sense to give cluster-scoped auth to a single user (or closely aligned users) sharing a WDS. Multiple uncoordinated users on the same WDS normally would not have such auth. It would be nice to have good performance for both of these scenarios.

adding here information just to verify we are all aware -
In kubernetes, it's possible to give users access only to their namespace and access to create a specific cluster-scoped object. for example, a user may have access to create anything in his namespace (admin of that namespace) and access to create BindingPolicy objects.
the option to create a specific cluster-scoped object doesn't make this user an admin or a super-user.
this is not a general cluster-scoped auth but a very specific one.

@dumb0002 These are surprising plots, particularly the grouping of latency values exactly on one second intervals for ks021. To disambiguate delays due to bundling+transport+klusterlet submission from delays due to K8S deploying the pods it could be interesting to see ks020 and ks021 plots of:
bundling+transport+klusterlet = t_pod-wec-creation - t_pod-wds1-creation

and
kubernetes deployment = t_pod-wec-completion - t_pod-wec-creation - 20

@eaepstein, the equation that I used above is already removing  delays due to K8S deploying the pods - that's achieved by subtracting the alpha parameter

I notice that when running the stress test and metrics together, kubectl reports for wec1:
stress-pod-3865   0/1     Pending             0          0s
stress-pod-3865   0/1     Pending             0          0s
stress-pod-3865   0/1     ContainerCreating   0          0s
stress-pod-3865   1/1     Running             0          1s
stress-pod-3866   0/1     Pending             0          0s
stress-pod-3866   0/1     Pending             0          0s
stress-pod-3866   0/1     ContainerCreating   0          0s
stress-pod-3866   1/1     Running             0          1s

Delays from wec creating to running could be much longer in burst testing.

@eaepstein, that's expected that burst workload will have a high latency for pod creation in the wecs when compared to the sequential workload generation

adding here information just to verify we are all aware - In kubernetes, it's possible to give users access only to their namespace and access to create a specific cluster-scoped object. for example, a user may have access to create anything in his namespace (admin of that namespace) and access to create BindingPolicy objects. the option to create a specific cluster-scoped object doesn't make this user an admin or a super-user. this is not a general cluster-scoped auth but a very specific one.

Actually no, I was not aware that namespaced-scoped users would  still be able to create a BP. That is good to know.

I am puzzled by one thing in the data shown in the opening comment. My recollection of the reports directly from @dumb0002 for KS 0.20 (or was it KS 0.21?) is that WDS-to-WEC latency is higher and more variable because the klusterlet operates periodically with a fairly large period. The reports from @dumb0002 showed step-by-step breakdowns; one of the steps was from ITS (formerly IMBS) to WEC and that shows the latency due to the klusterlet's behavior. My memory may be wrong. I do not have a pointer to such a document handy. I think it would be great to look at one of those reports.

Does each entry in the bundled manifestwork have a create time?

Yes. I am testing release 0.21.2-rc2, and in the ManifestWork objects I see that each wrapped workload object retains its metadata.creationTimestamp (that is a standard bit of Kubernetes object metadata).

Do entries also have the ID of the associated workload objects to be able to measure transport times?

Yes, it would be problematic if not. In Kubernetes an object is identified by name and namespace (among objects of the same kind), and user code knows this and, in general, works with this.
@dumb0002 has been producing reports that break down the end-to-end times into individual steps, relying on standard Kubernetes metadata.

@nirrozenbaum wrote:

If that's ok with all of you, give me around two weeks and I'll try to come up with stress test setup that represents KS usage better.

I would differentiate between defining a stress test and implementing a stress test. I would be ready to discuss stress test definitions without waiting for implementation. After we get our current bugs out and customization done, of course.

if in a representative cases we'll see that wrapping individual objects performs better I will change the implementation to do that.

How do we really know what are representative cases? We are designing for a range of possible uses. Remember edge comuting? The MCAD stack is not that. Do we have to pick just one? I think that we are in too early a stage to rule out use cases that we have considered. OTOH, I think it is fine to focus our current work on the use cases that have the most immediate traction.
It seems to me that if we find that we want to support a mixture of use cases, in which some work best with bundling and some work best with individual object wrapping, then this is something we could definitely do. I suspect that in such a world it would be known at deployment time which is the better way to operate; in that case the generic transport controller code could easily take a command line flag telling it which way to go.

Users expect to get the same latency/response behavior regardless of the system load or scale.

Users expect ponies too. I think we have to settle for satisfying reasonably educated users.

I am running out of time, have not read all the comments yet. I will leave a few thoughts of mine on usage scenarios.
I think the orientation of multiple "users" sharing a WDS is not very compelling. It reminds me of time-sharing. I am more drawn to thinking about how an organization uses a larger system that includes KubeStellar. For example, in edge computing scenarios. There the number of WECs is very large. And with a very large number of WECs, it is practically guaranteed that there will always be some coming and going. The rate of change in the population of WECs will likely be greater than the rate of change in the workload and BidingPolicies. In such a scenario, KubeSteller is not being used by a bunch of independent users. It is used by a cooperating collection of teams collaborating on managing what is going on at the edge.
Another sort of usage scenario is 1:1 dispatching of some sort of work among multiple execution clusters. The MCAD stack is an extremely small and simple version of this. Some of us remember working with the Octopus team on a little more general version. Some higher level thing occasionally drops another workflow into a WDS. Some other central controller factors the workflow into pieces and decides which execution cluster each piece should go to. An execution cluster can potentially handle multiple workflow pieces at once, depending on the cluster's capacity and the needs of the pieces. In these scenarios, there can be one standing BindingPolicy for each execution cluster and the dispatched work flows through the KubeStellar pipes to the WECs. To get down to numbers worth calculating with, you have to get into application specifics. An ML training job is going to be so big that it will entirely consume a WEC. I am less clear on the data prep steps, but they are going to be pretty big too. OTOH, individual inferencing jobs may to too small to make sense for dispatching via KubeStellar. Don't look to me for numbers here, ask people who know. Don't only look at ML.

Also: KubeStellar is not "transparent multi-cluster". It is intentional and explicit management of workload in remote clusters.

@nirrozenbaum wrote:

If that's ok with all of you, give me around two weeks and I'll try to come up with stress test setup that represents KS usage better.

I would differentiate between defining a stress test and implementing a stress test. I would be willing to discuss stress test definitions without waiting for implementations. After we get our current bugs out and customization done, of course.

@MikeSpreitzer I agree. we can try to find some time to define stress test after current work is completed.


Add -open and -close options

Hi, I just added the -open and -close options to make sure that sometimes open and close need to be specified explicitly.

I also add two signals to specify close and open:
SIGUSR2 for open, and SIGRTMIN+3 for close, which is user defined real-time signals.

Could you also describe new flags and signals in README.md?

Done!

Sorry for the direct copy of --help, probably because I used kitty when compiling
main.go:173
var term = flag.String("term", defaultTermIfBlank(os.Getenv("TERM"), "foot"), "Terminal emulator")


Print progress of FindMergeCommits

@benedikt-schesch Sorry, I hit "Squash and merge" by accident.  If you have any comments on the code, let me know.


Fixed up-to-date hash values in badges of EN docs

Fixes #97.
I'm sorry, I pushed hash values that were changed by git rebase in #97.
This will probably work.

Changes proposed in this PR:

change hash values in badge of .md files

@mtshiba

Thanks!


Test utils before splitting them

Partially fixes #129

Codecov Report

Merging #148 (e5f164e) into develop (1d4b21c) will increase coverage by 13.47%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##           develop     #148       +/-   ##
============================================
+ Coverage     2.76%   16.24%   +13.47%
============================================
  Files           12       12
  Lines         1265     1256        -9
============================================
+ Hits            35      204      +169
+ Misses        1230     1052      -178

Impacted Files
Coverage Δ

src/utils.py
34.32% <91.66%> (+34.32%)
:arrow_up:

config/constants.py
100.00% <0.00%> (+100.00%)
:arrow_up:


Context API

Obviously React is getting a new Context API, but you have to support more than 16.3 with Formik so you won't be able to use it. I asked Dan Abramov on Twitter and he recommended using React Broadcast to polyfill it for older versions. What about adding this to formik-effect/formik instead of using the old context api? The primary reason is this:
const MyPureComponent extends React.PureComponent {
  render() {
    return this.props.children;
  }
}

const MyForm = (props) => (
  <Formik
      onSubmit={values => console.log(values)}
      initialValues={{ firstName: '', lastName: '', email: '' }}
      render={props =>
        <Form className="whatever">
          <MyPureCompoent>
            <Effect onChange={(currentFormikState, nextFormikState) => {
                 // This will never receive changes as PureComponent props never change
                 // and wont pass them down to their children
              }}
            />
          </MyPureComponent>
          <Field name="firstName" placeholder="First Name" />
          <Field name="lastName" placeholder="Last Name" />
          <Field name="email" type="email" placeholder="Email Address" />
          <button type="submit">Submit</button>

        </Form>}
    />
);

Unfortunately moving to React 16.3 is going to be somewhat painful for Formik because it will introduce another component for each primitive....potentially breaking everyone's enzyme tests. I attempted this move a few weeks ago in a PR with create-react-context. The problem we are going to need to solve for is that Formik's context provider (<Formik>) is also a render prop. The current API does not allow that because the <Provider> it calls React.Children.only() if I recall.


[ layer ] Enable mha gtest and match version @open sesame 06/04 13:01

Current mha layer at nntrainer/layer is not for general use, but implemented solely for LLaMA support.
In order to run unittest for mha layer:

rollback current mha layer to previous one
move current implemenation to Application/LLaMA
enable unittest

Self evaluation:

Build test:     [X]Passed [ ]Failed [ ]Skipped
Run test:     [X]Passed [ ]Failed [ ]Skipped

:memo: TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2617. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

LGTM

But I wonder... won't this PR hinder some dependencies in other LLaMAs using mha? Just asking.

Currently, MHA layer in the layers directory is rollbacked. I would like to know if you tested the LLaMA Application with this a custom MHA layer as in this PR. Please confirm it first.


Fix recipe_wenzel14jgr

Description

The data nbp_Lmon_HadGEM2-ES_esmHistorical_r1i1p1.nc is no longer available on ESGF (see #2408).
What I did:

First attempt by removing only removing the missing data from the recipe (HadGEM2-ES, exp: esmHistorical, variable: nbp) but that triggered a diagnostic error for diag_gammaHist_Fig3and4. I think the exact same models are needed for some of the variables.

Second attempt by removing (HadGEM2-ES, exp: esmHistorical) for all variables of the diagnostic diag_gammaHist_Fig3and4 but I got an error in the next diagnostic diag_gammaLT_5 which expects data from the HadGEM2-ES model.

Third successful attempt by removing (HadGEM2-ES, exp: esmHistorical) for all variables of the diagnosticsdiag_gammaHist_Fig3and4 and diag_gammaLT_5

Related to #2408 and #2535
Link to documentation: https://docs.esmvaltool.org/en/latest/recipes/recipe_wenzel14jgr.html

Before you get started

[x] ☝ Create an issue to discuss what you are going to do

Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

[x] 🛠 This pull request has a descriptive title
[x] 🛠 Code is written according to the code quality guidelines
[ ] 🛠 Documentation is available
[ ] 🛠 Tests run successfully
[x] 🛠 The list of authors is up to date
- [ ] 🛠 Any changed dependencies have been added or removed correctly
[ ] 🛠 All checks below this pull request were successful

New or updated recipe/diagnostic

[x] 🧪 Recipe runs successfully
[ ] 🧪 Recipe is well documented
[ ] 🧪 Figure(s) and data look as expected from literature
[x] 🛠 Provenance information has been added

New or updated data reformatting script
- [ ] 🛠 Documentation is available
- [ ] 🛠 The dataset has been added to the CMOR check recipe
- [ ] 🧪 Numbers and units of the data look physically meaningful

To help with the number of pull requests:

🙏 We kindly ask you to review two other open pull requests in this repository

@axel-lauer and @schlunma please let me know if and how the documentation should be updated. I'm not familiar enough with this recipe to tell myself.

Thanks @remi-kazeroni for fixing this! The documentation does not explicitly mention any models, so I would leave it as it is.


MapLibre - Vue3 - Vite Migration v2

Hi everyone, this is my 2nd attempt to migrate the internal UI Viewer project.
All the UI (dev) libraries have been updated to the most recent versions.
Mapbox -> MapLibre
Vue2 -> Vue3
Vue Cli -> Vite

Pull Request Test Coverage Report for Build 2a4c58b27-PR-926

0 of 0   changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 46.894%

Totals

Change from base Build 62bfffe1d:
0.0%

Covered Lines:
6553

Relevant Lines:
13974

💛  - Coveralls

@iwpnd it looks like all your comments have been addressed. Would you mark the PR approved if you don't have any additional change requests?

my bad, wasn't aware that I was appointed a reviewer and was blocking this


sentry_sdk.capture_message() does nothing within a FastAPI route

How do you use Sentry?
Sentry Saas (sentry.io)
Version
1.32.0
Steps to Reproduce
I'm working with a FastAPI application and when I attempt to call sentry_sdk.capture_message() from within a request context, nothing is logged in Sentry.
Here is a minimal app to reproduce the issue:
import sentry_sdk
from fastapi import FastAPI
import os

sentry_sdk.init(
    dsn=os.getenv('SENTRY_DSN'),
)

app = FastAPI(title="Sentry Test")

sentry_sdk.capture_message("TEST MESSAGE")

@app.get('/')
def example():
    sentry_sdk.capture_message("TEST MESSAGE (IN ROUTE)")
    return 'Ok'

Running the previous snippet with SENTRY_DSN=XYZ uvicorn app:app, I notice the following:

TEST MESSAGE will appear in Sentry when the app starts
If I navigate to the configured route (http://localhost:8000/), the second message (TEST MESSAGE (IN ROUTE)) does not appear in Sentry.

I observe that the second message will appear if I set auto_enabling_integrations=False when initializing sentry_sdk, however I would prefer to gain the benefits of the FastAPI integration.
Below is the output from pip freeze --all:
annotated-types==0.6.0
anyio==3.7.1
certifi==2023.7.22
click==8.1.7
exceptiongroup==1.1.3
fastapi==0.103.2
h11==0.14.0
idna==3.4
pip==21.2.4
pydantic==2.4.2
pydantic_core==2.10.1
sentry-sdk==1.32.0
setuptools==58.1.0
sniffio==1.3.0
starlette==0.27.0
typing_extensions==4.8.0
urllib3==2.0.6
uvicorn==0.23.2

Expected Result
I expect to receive a message, TEST MESSAGE (IN ROUTE), in Sentry when a user navigates to http://localhost:8000/
Actual Result
No message appears in Sentry when a user navigates to http://localhost:8000/

@sentrivana here is the full debug trace:
 [sentry] DEBUG: Setting up integrations (with default = True)
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.aiohttp.AioHttpIntegration: AIOHTTP not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.boto3.Boto3Integration: botocore is not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.bottle.BottleIntegration: Bottle not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.celery.CeleryIntegration: Celery not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.django.DjangoIntegration: Django not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.falcon.FalconIntegration: Falcon not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.flask.FlaskIntegration: Flask is not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.httpx.HttpxIntegration: httpx is not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.pyramid.PyramidIntegration: Pyramid not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.rq.RqIntegration: RQ not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.sanic.SanicIntegration: Sanic not installed
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.sqlalchemy.SqlalchemyIntegration: SQLAlchemy not installed.
 [sentry] DEBUG: Did not import default integration sentry_sdk.integrations.tornado.TornadoIntegration: Tornado not installed
 [sentry] DEBUG: Setting up previously not enabled integration argv
 [sentry] DEBUG: Setting up previously not enabled integration atexit
 [sentry] DEBUG: Setting up previously not enabled integration dedupe
 [sentry] DEBUG: Setting up previously not enabled integration excepthook
 [sentry] DEBUG: Setting up previously not enabled integration logging
 [sentry] DEBUG: Setting up previously not enabled integration modules
 [sentry] DEBUG: Setting up previously not enabled integration stdlib
 [sentry] DEBUG: Setting up previously not enabled integration threading
 [sentry] DEBUG: Setting up previously not enabled integration fastapi
 [sentry] DEBUG: Setting up previously not enabled integration redis
 [sentry] DEBUG: Did not enable default integration redis: Redis client not installed
 [sentry] DEBUG: Setting up previously not enabled integration starlette
 [sentry] DEBUG: Enabling integration argv
 [sentry] DEBUG: Enabling integration atexit
 [sentry] DEBUG: Enabling integration dedupe
 [sentry] DEBUG: Enabling integration excepthook
 [sentry] DEBUG: Enabling integration logging
 [sentry] DEBUG: Enabling integration modules
 [sentry] DEBUG: Enabling integration stdlib
 [sentry] DEBUG: Enabling integration threading
 [sentry] DEBUG: Enabling integration fastapi
 [sentry] DEBUG: Enabling integration redis
 [sentry] DEBUG: Enabling integration starlette
 [sentry] DEBUG: Setting SDK name to 'sentry.python.fastapi'
 [sentry] DEBUG: Sending event, type:null level:info event_id:d72189e28d1a4484af8b7e5a2d6530ae project:4505347059351552 host:o4505347056599040.ingest.sentry.io
 [sentry] DEBUG: [ASGI] Created transaction (continuing trace): <Transaction(name='http://<IP_ADDRESS>:8000/', op='http.server', trace_id='5e35bcd184a74a939070732c76bfe32c', span_id='bb3c99889c5a2ea2', parent_span_id=None, sampled=None, source='url')>
 [sentry] DEBUG: [ASGI] Set transaction name and source on transaction: 'http://<IP_ADDRESS>:8000/' / 'url'
 [sentry] DEBUG: [Profiling] Discarding profile because transaction is discarded.
 [sentry] DEBUG: [ASGI] Started transaction: <Transaction(name='http://<IP_ADDRESS>:8000/', op='http.server', trace_id='5e35bcd184a74a939070732c76bfe32c', span_id='bb3c99889c5a2ea2', parent_span_id=None, sampled=False, source='url')>
 [sentry] DEBUG: [FastAPI] Set transaction name and source on scope: / / route
 [sentry] DEBUG: [Profiling] updating active thread id to<PHONE_NUMBER>
 [sentry] DEBUG: Sending event, type:null level:info event_id:4ca53503373b424bbc5d0124990f7a49 project:4505347059351552 host:o4505347056599040.ingest.sentry.io
 [sentry] DEBUG: Discarding transaction because sampled = False
 [sentry] INFO: Discarded session update because of missing release
 [sentry] DEBUG: [ASGI] Created transaction (continuing trace): <Transaction(name='http://<IP_ADDRESS>:8000/favicon.ico', op='http.server', trace_id='d21c6cd822b84b308ec25c5dcb1e9be3', span_id='99afbc0549a1f332', parent_span_id=None, sampled=None, source='url')>
 [sentry] DEBUG: [ASGI] Set transaction name and source on transaction: 'http://<IP_ADDRESS>:8000/favicon.ico' / 'url'
 [sentry] DEBUG: [Profiling] Discarding profile because transaction is discarded.
 [sentry] DEBUG: [ASGI] Started transaction: <Transaction(name='http://<IP_ADDRESS>:8000/favicon.ico', op='http.server', trace_id='d21c6cd822b84b308ec25c5dcb1e9be3', span_id='99afbc0549a1f332', parent_span_id=None, sampled=False, source='url')>
 [sentry] DEBUG: Discarding transaction because sampled = False
 [sentry] INFO: Discarded session update because of missing release

There are two Sending event... entries, the first corresponding to startup and the second corresponding to the endpoint message, however I still only receive the first message in Sentry

Stupid question, but you've ruled out filters in Sentry being responsible for the second message not popping up (e.g. the issue was marked as resolved at some point and is thus not showing up as a new issue)?

@sentrivana It turns out that this isn't a stupid question at all. There was an Inbound Data Filter that was filtering out all localhost traffic. Disabling this allowed me to test locally 🤦
Thanks a bunch for helping me debug this and apologies for the oversight on my side.

@jarednieder No worries! Happy that it's working now. 👍🏻


Generate tilemapresource.xml

Can not generates tilemapresource.xml.

Hello. Currently I don't create any other files, except tile images. It's a good idea to add creation of such an xml file, I'll consider working on it for 2.1.x update!

It would be great if could create this file. I tried to publish the tiles to GeoServer, it was missing tilemapresource.xml and could not published.

Roadmap for feature:
Core:

[ ] Implement classes for serialization/deserialization;
[ ] Write all precondition checks;
[ ] Complete xml-docs;

Tests:

[ ] Complete tests for this feature and fix errors if any;
[ ] Update code coverage;

Console:

[ ] Add an arument to create tilemapresource.xml after creating tiles;

GUI:

[ ] Add an option to create tilemapresource.xml after creating tiles;

Docs:

[ ] Update CHANGELOG.md;
[ ] Update Console docs;
[ ] Update GUI docs;
[ ] Rebuild github pages docs with docfx;

Thanks，Very helpful


Validate credentials before using them

By accident I forgot to enter my credentials to ~/.release-droid/credentials:
jira_username=
jira_password=

This lead to a failure when releasing to jira.
Before starting, release-droid should verify that non-empty credentials are available.

If you forget to add credentials, the release droid asks you to input them interactively via console. So this is intended not to check that jira_username/jira_password/etc in the file because not all of us want to keep credentials in the file.


Prepare for review of the web-browser <-> model interface

Describe the current interface between the UI and the model.
In particular, how are the interface elements populated from the model and how are changes in the interface propagated to the model simulation configuration?

The complexity is low.  One "run" method and a couple of files.  No need to document at this time.
URL paths between browser and interface is cleaned up and commented.


Preloaded cryptocards in retail

One of the main characteristic of smartcard-based hardwallets is their low cost. This kind of cards can be a very good support to trigger adoption of crypto-currencies for those who don’t want to go through an exchange to buy crypto.
Some paper-wallets type of cards like ether.cards https://ether.cards/ offer this type of convenience by providing an already generated wallet, and will probably soon offer the same type of cards with a pre-paid amount of crypto on the wallet. We think that using a smartcard instead of a paper wallet for this type of scenarios could provide several security and convenience advantages.
In our current simple deployment scheme, the cards are delivered to the user without any wallet key pair. There is neither any specific mechanism already defined to uniquely identify each card.
There are two use cases that would make sense :
1/ prepaid card
Our card is advertised as holding 50$ (example) of eth. The user buys this card in with 50$ of fiat in a store or online. When the user receives this card, he is guided (ideally through a secure tap-to-download an app feature) through an application (e.g. Status app) to activate his card, and his wallet is automatically filled with 50$ worth of crypto at the current exchange rate.
2/ empty card
Same scenario but sold in store. The user picks up a blank card, goes to the cashier, asks for the Fiat amount he wants to put on the card and the crypto type (« let me get 35$ of eth, please 🙂 « ). The cashier has a dedicated app on his phone, or a dedicated POS, cashes in the 35$ and provision the crypto in the card.
Let’s brainstorm on what are the difficulties, and come up with a full system solution.

In scenario #1 ideally the card is a Faraday cage  (e.g. wrapped in aluminium foil) and sealed - to prevent trolls from bricking the cards.
Would also like to see scenario #1 with DAI instead of ETH (or a combination of both ETH for paying gas and DAI for spending)

Since scenario 1 can be most likely supported by a smart contract, it would nice to bring some smartcontract guys in the discussion.

I think scenario 1 could be supported by something similar to the Hackathon Registration Dapp, meaning, if someone wants to buy a card with 50$, besides the card, he would receive a QR code that he could scan later in a dedicated dapp for the hardware wallet.

I love @richard-ramos option!

sounds like a support nightmare

maybe an idea would be, instead of a QRcode to have a special applet for provisioning the ETH? It would be an applet with a private key (the public key would be known by the dapp) which could sign challenges coming from said dApp. QRCode would require unique code to printed on each package (somewhere were it cannot be seen without opening)

Is the smart card able to sign a message that uniquely identifies the card?

yes, the card could sign its own unique ID

Let's please try to avoid QR codes ..-)

@bitgamma If that's the case, I think the idea is doable. A smart contract could be created to give the $$ and keep track that it did that transaction when it received a message with a valid unique ID.
Are these IDs registered somewhere?

Indeed. We need to register somewhere the smart card IDs (individually or via merkle root) to guarantee it's a card that's allowed to withdraw funds. Can the fund amount change depending on the card? (Card A allows you to access 10$,  Card B, 50$)

Registering them individually is the best solution if not too resource
expensive.
However, strictly speaking and only if resources/logistics do not allow
tracking all issued cards, it would be possible to just register the public
key for each category ($10, $20, etc). This would work because only our
cards would have the correct private key to generate the claiming signature
and we can trust our own card to not generate multiple IDs. Of course the
claimed ID must be registered in any case to avoid the same card claiming
multiple times. The disadvantage of this solution however is that if the
private key is leaked, it becomes trivial to generate arbitrary IDs and
signatures.
If we keep track instead of all issued cards it becomes much more difficult
to attack even if the private key leaks, since it would become a brute
force attack probably more expensive than minining.
Richard Ramos<EMAIL_ADDRESS>23 ноября 2018 г. 20:38:10 написал:

Indeed. We need to register somewhere the smart card IDs (individually or
via merkle root) to guarantee it's a card that's allowed to withdraw funds.
Can the fund amount change depending on the card? (Card A allows you to
access 10$, Card B, 50$)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.


[Manifest] Mark digest as required

Clarifies that digest is required per Issue #103
Signed-off-by: Rob Dolin<EMAIL_ADDRESS>Closes #103

LGTM

LGTM


Local server setup in single board computer or traditional system.

Is your feature request related to a problem? Please describe.
Currently the subscription charges of running virtual machine on MS Azure for this project isn't much budget friendly.
Describe the solution you'd like
To make the server locally installed in a single board computer or traditional system.
Additional context
Linux will be best in single board computer and with network and firewall setup to enhance data security and privacy.

"I'd like to work on this issue. Could you please assign it to me?


Add error to UnsInt from_hex when hex too big

Fix panic on from_hex with big strings
Type of change
Please delete options that are not relevant.

[x] Bug fix

Checklist

[ ] Linked to Github Issue

Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 72.02%. Comparing base (dfe4f0e) to head (030fbb6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #880      +/-   ##
==========================================
+ Coverage   72.01%   72.02%   +0.01%
==========================================
  Files         149      149
  Lines       33377    33393      +16
==========================================
+ Hits        24036    24052      +16
  Misses       9341     9341

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


Allow to preserve an unknown properties

Precheck
If needed to control an "unknown" properties (in terms of vscode linter), clean-css doesn't provide any option to do it.
'skipProperties' option doesn't resolve this.
It's can be useful if dev want to control svg (implemented in html document directly) and it's child elems from css.
Environment

clean-css version - npm ls clean-css: 4.2.3
node.js version - node -v: 16.13.1
operating system: Win10 x64 (21H1, build 19044.1469)

Hey @Xardion495 - do you mean there should be a switch which lets clean-css treat any non-standard CSS properties as valid ones and simply keep them as they are? I believe it works in clean-css 5.x - can you at least give it a try at https://clean-css.github.io/ ?

I believe it works in clean-css 5.x - can you at least give it a try at https://clean-css.github.io/ ?

Hi. Yeah, you are right, it works at https://clean-css.github.io
I using gulp with clean-css wrapper plugin which depends on old clean-css version.
So i used solution from here: https://github.com/clean-css/clean-css/pull/1173 and the problem is no more.
Thanks for responses.

Cool, no problem!


adding commands ps and collect to docs

Fixes #17220

@sywhang When were these new subcommands introduced?  Do we need any applies to info update?

@sdmaclea I don't remember the exact version, but since it's relatively easy for customers to just update to the latest version I think it's fine to leave out applies to info?


Missing display name for America/Fort_Wayne,Antarctica/Troll

Hello,
I actually miss "en-US" display name for these two Iana codes:
America/Fort_Wayne and Antarctica/Troll
Would it be possible to add it ?
Version: 5.0.1
Thank you

Antarctica/Troll is un-mappable - it has no Windows equivalent (it's the only one).  So there's no Windows display name for it.
America/Fort_Wayne should work, as it's an alias to America/Indiana/Indianapolis (or America/Indianapolis by CLDR) and maps to US Eastern Standard Time.  It should have the Windows display name (UTC-05:00) Indiana (East).  Does it not work for you?

This was fixed in 6.0.0.  Thanks


ARROW-4977: [Ruby] Add support for building on Windows

I've rebased on master and enabled test for Ruby on AppVeyor.
AppVeyor CI is passed. Test for macOS on Travis CI is failed. I think that this isn't related. I restarted the job.


[REQUEST] Add a release of native ob2 to run without installing .net desktop runtime

Please add another release of native ob2 to run without installing .Net Desktop Runtime
you can also do same about browser version
thanks

What's the point though? It would just add some nuisance to the updater. Is there some reason why you cannot install .NET 6 directly?

What's the point though? It would just add some nuisance to the updater. Is there some reason why you cannot install .NET 6 directly?

because sometimes i have an rdp without admin access and i can install nothing. and also it's useful in other cases


Respect passed port when creating Kubernetes provider

A superficial attempt at fixing #13306, just for Kubernetes.
Depends on #13317 for tests, see only second commit for the fix here.
TODO: pretty sure same affects Hawkular and Datawarehouse providers;
and might affect many other virtual fields?
https://github.com/ManageIQ/manageiq/search?utf8=✓&q=default_value_for
Centralized the default port logic in Kubernetes::ContainerManagerMixin — this simplification might be worth it even if we find generic solution.
Verified that this preserves the DEFAULT_PORT consts that UI uses:
https://github.com/ManageIQ/manageiq-ui-classic/blob/a9d1ccace8ae1967c27d3250d7847e24714de32a/app/controllers/ems_common.rb#L895-L900
however I'm not sure that UI logic is doing anything?  It's in get_form_data parsing incoming params, but UI doesn't allow pressing [Validate] nor [Save] with unfilled port.  What would be desired is for Add New Provider to fill port field depending on chosen provider type, but AFAICT this wasn't happening before either (always prefilled with 8443).

Do we need this default at all?!
Perhaps in API if creating without missing port?  TODO: test this.

@miq-bot add-label providers/containers, api, bug, wip

@dkorn Please review.

@dkorn @simon3z While this doesn't address the root cause, it fixes the immediate bug and simplifies our code a bit, so would be good to merge anyway.
@miq-bot remove-label wip

LGTM :+1:
@cben Is there a separate PR for the tests? (commit https://github.com/ManageIQ/manageiq/pull/13317/commits/3638c624c205128484c77bd901154b66df3ccf52) if not, I'd move the port tests to this PR.
regarding the default ports API vs UI, IMHO I think both should have the same logic, meaning the port should be required in API requests.

That tests commit originated in #13317 and is by now shared with this PR, #13400, and some other upcoming work...  I'm waiting for at least one PR to land it :-)

regarding the default ports API vs UI, IMHO I think both should have the same logic, meaning the port should be required in API requests.

That would mean we don't actually need any default here?  We'd only need the UI to pre-fill the correct default in empty form.
BTW, the default here is only for default_endpoint's port, doesn't give hawkular a default port.  UI pre-fills 443; also if the endpoint is missing we default at runtime to metrics_port which settings.yml sets to 5000 (?) for kubernetes and nothing for openshift.
Tested now what API does — sent connection_configurations with 2 endpoints, omitting both ports.
=> "default" endpoint was stored with 8443, "hawkular" with nil (there is no explicit code checking that, but URI:HTTPS with nil port builds uri without :port part, which means 443, so it'll probably work...).
So requiring port in API might break compatibility.


Create Documentation

Is #84 a sufficient solution?

In my opinion yes!
Really neat with fuget :)
Going forward we need to create a guide for the framework with examples and documentation references. I'll make a new case for that and close this one.


Custom Redirects Manager redirect URL's are not working for the expired pages

I'm having a problem when redirecting an expired page to another page in Custom Redirects Manager.
All redirects are working in Custom Redirects Manager if the content for both the original page and the target page has not expired. If the content has expired for the original page, then either get “404 error” message or “redirect too many times error” message shows even if a not expired redirect page is configured in Custom Redirects Manager.
It seems like expires page in Epi server CMS having problem communicating with "Custom Redirects Manager" configured redirect URLs when the original page has expired. However, if they configure redirects with shortcuts ( can configure in expired page settings Tab) then it redirects to the configured link.
I just need to know why expired page redirects not working in “Custom Redirects Manager”. Is it a bug or is it normal that “Custom Redirects Manager” not handle expired page redirects ? . Is that the only option to configure redirects with shortcuts in page setting for such expired pages ?

Versions in CMS

I noticed the same behavior.
Expired pages return a 410 response.
I created a PR that makes it configurable if the NotFound Handler will mange redirects on 410 requests, not just 404 responses.
https://github.com/Geta/geta-notfoundhandler/pull/155


kvstoremesh: correctly remove cached data upon cluster disconnection

Currently, a comment attached to the Remove method claims that the cached data would have disappeared due to lease expiration. However, the lease is never expected to expire, as we use a single local client for all remote clusters. Additionally, depending on lease expiration would leave a significantly long race condition window in which stale data would still be present if the same cluster were to be reconnected.
In most cases, stale cached data is not a problem, as the etcd instance is stateless. However, it can create problems if the same cluster is first disconnected and then subsequently reconnected, without restarting the clustermesh-apiserver pod in between. Indeed, in this case we would have leftover entries that would not be cleaned up before reconnecting.
Let's address this issue by explicitly deleting the cached data upon cluster disconnection. This operation is performed synchronously (i.e., blocking other connection/disconnection operations) to prevent race conditions causing the deletion of incorrect data. Yet, to avoid blocking forever in case of errors, we only allow a limited number of retries, and give up with a loud error if they all fail.

Fixes: #issue-number
Correctly remove data cached by KVStoreMesh for a given cluster when disconnecting from such cluster

/test

one nit, one bigger comment. Anyway, if we figure out these corner cases, I would encourage to do it behind a hidden flag, just in case, we need to disable it due to some race-condition.

Thanks! I've added the hidden flag, PTAL.

/test

Converting back to draft, because this change introduces a possible race condition when disabling KVStoreMesh, and causes flakiness in the Cilium Cluster Mesh upgrade workflow.
The reason being that disabling kvstoremesh via helm:

Switches the Cilium Agents clustermesh configuration from the local cache to the remote etcd;
Deletes the kvstoremesh container, causing the restart of the clustermesh-apiserver pod;
Deletes the kvstoremesh clustermesh configuration.

However, if the old kvstoremesh container is still running when the kvstoremesh configuration removal is propagated by the kubelet, it will interpret it as a cluster disconnection, hence triggering the newly introduced draining of the cached data. However, if the Cilium Agents are still watching this replica at that time (i.e., they have not yet observed the corresponding clustermesh configuration change), they will observe deletion events for keys that are actually still valid, hence causing the disruption of existing connections.

The above issue is addressed waiting for a grace period (hardcoded to 3 minutes) before triggering the deletion of the cached data. This time window is meant to allow all Cilium agents to disconnect from KVStoreMesh in the meanwhile, both to reduce the overhead on etcd (as the deletions would not be watched by anyone) and prevent the race condition when disabling KVStoreMesh altogether. To address the fact that the removal process now takes longer, the first commit makes this operation asynchronous, so that additions and removals of other cluster configurations can be processed in parallel. Differently, the recreation of a configuration for a cluster which is being removed is queued and replayed once the removal completes, to still ensure the correct ordering of the operations.

/test

/test

Overal makes sense, but I would question whether this part:
 To address the fact that the removal process now takes longer, the first commit makes this operation asynchronous, so that additions and removals of other cluster configurations can be processed in parallel. Differently, the recreation of a configuration for a cluster which is being removed is queued and replayed once the removal completes, to still ensure the correct ordering of the operations.

is necessary. Isn't that also increase the load on agent as we will be doing removal in parallel now?
I'm honestly on the edge what would be better/safer. I don't think we should have super fast handling for cluster removal even if it blocks adding new clusters, if it introduces significant mental complexity around tombstones and removal of clusters.

Generally speaking I was also a bit on the fence about the best approach here (and I'm not a big fan of the new logic, which is less straightforward). Yet, I think that there are a couple of main aspects justifying the asynchronous deletion, although a bit more complex.
Before that, as a quick recap, the logic to observe configuration changes is used both by Cilium agents and kvstoremesh (and also by the operator when endpointslice synchronization is enabled, but that's akin to the agents). In both cases, upon new cluster detection (i.e., appearance of a new configuration), we start a controller that runs in background, establishes the connection to the target etcd instance, retrieves the cluster configuration (i.e., the one containing the ClusterID) and eventually calls the agent/kvstoremesh specific logic to mainly start the appropriate watchers (the agents perform the ClusterID validation here). Upon configuration removal, instead the logic stops the above controller (i.e., stopping the watchers and closing the etcd client) and then calls the specific removal logic. Agents essentially emit a synthetic deletion event for all known entries, to drain them. Differently, with this PR, kvstoremesh triggers the cleanup of the prefixes (with the initial grace period to avoid the issues discussed previously). All the disconnect and removal part was historically performed synchronously, and is changed by the first commit.
Back to the reasons,

Making the deletion asynchronous brings it on par with the connection to a new cluster, which is also performed asynchronously via a controller;
Blocking the configuration of a new cluster for a relatively long period of time would be confusing for users. Let's consider the kvstoremesh case with the 3 minutes grace period. Supposing a user disconnects 5 clusters and connects a new one, the new one may not be reconciled for 15 minutes, giving the impression that something didn't work. The order in which the cluster configuration events are processed is also undeterministic (it depends on the inotify), which makes it even worse.
Most importantly, it would lead to asymmetry between the agents and kvstoremesh. Back to the previous example, and besides the fact that ordering may be different, the removal on the agent side would be relatively fast, while the grace period would slow down the kvstoremesh cleanup quite a lot. That would mean that agents would attempt connecting to kvstoremesh for the new cluster way earlier than when kvstoremesh could start performing the synchronization, hence entering into a backoff loop as the cluster configuration has not yet been propagated.

Isn't that also increase the load on agent as we will be doing removal in parallel now?

Agents do need to process the synthetic deletion events upon cluster removal, so this can potentially increase the load a bit. However, that's not dissimilar to the initial listing upon agent startup, which is also performed in parallel, and it is arguably more demanding (both in terms of operations, and because it involves all the clusters the agents are connected to). As for kvstoremesh, instead, the expectation is that all agents have already stopped the watchers at that point, so the deletion of the prefixes should be relatively cheap.

This was mostly triggered due to the fact that I'm not really sure if using name vs clusterID in tombstones is a good idea.

IMO cluster names are correct there, because they correspond to the names of the configurations. Hence, that ensures that subsequent creations and removals of the same configuration file are handled correctly, and coalescenced as appropriate. Differently, ClusterID are only retrieved after connecting to the etcd instance.

I would say that two examples that you gave:
agents shutdown while the removal of one or more clusters is being processed. In this case, it is possible that the cleanup doesn't complete in time with such a short termination grace period, as the agents would need to finish processing all the synthetic deletion events. However, this was already the case earlier, and it is an extremely rare condition IMO.

and
Blocking the configuration of a new cluster for a relatively long period of time would be confusing for users. Let's consider the kvstoremesh case with the 3 minutes grace period. Supposing a user disconnects 5 clusters and connects a new one, the new one may not be reconciled for 15 minutes, giving the impression that something didn't work. The order in which the cluster configuration events are processed is also undeterministic (it depends on the inotify), which makes it even worse.

have probably a similar frequency of happening. In general, adding/removing clusters does not happen often, but once it happens, churn on nodes is not something very unlikely to happen during this time, nodes come and go all the time.

Yep, I agree with that. Currently the effect of possibly blocking the shutdown in that situation is mostly negligible due to the termination grace period being set to 1 second, but I agree that it could be worth an improvements if we decide to raise that period. One possibility could be using a context to interrupt the generation of the synthetic deletion events on shutdown (the GC on restart is expected to take care of them regardless).

/test

Nice, looks good.
A bit confusing for me was that onRemove uses a different context from drainJobFn though and we don't cancel drainJobFn in Stop() of func (cm *clusterMesh) Stop(cell.HookContext) error { If my understanding is correct these context are attached to two different Stop hooks, right? One from jobGroup thing, the other one to new rctx? I know it's still correct, but a bit confusing to me. Ideally, we would have rctx attached to jobGroup, no?
Just wanted to confirm my understanding, but lgtm otherwise.

Yes, the reason for the separate rctx is that the job gets registered earlier, and hence its stop hook is called later than clustermesh.Stop. Specifically, the job is automatically provided and registered by hive given that we retrieve it as a parameter here, and the resulting ordering is:
Start hooks:

  ...
  • *job.group.Start (kvstoremesh.kvstoremesh)
  • *kvstoremesh.KVStoreMesh.Start (kvstoremesh.kvstoremesh)
  • *common.clusterMesh.Start (kvstoremesh.kvstoremesh)
  ...

Stop hooks:

  ...
  • *common.clusterMesh.Stop (kvstoremesh.kvstoremesh)
  • *kvstoremesh.KVStoreMesh.Stop (kvstoremesh.kvstoremesh)
  • *job.group.Stop (kvstoremesh.kvstoremesh)
  ...

Another possible approach would be manually constructing the job group from a job registry, and registering it below the other ones, here. That's how I addressed it initially. However, it seemed to me that having an explicit "shutting-down" signal via the Remove context may be helpful as well to potentially interrupt the draining process in the agents, if needed.
I can revert to the other approach if you prefer that though. Another possibility could be propagating rctx into drainJobFn, and use that rather than the job context, although it doesn't seem great either to me. WDYT?

After an offline discussion with Marcel, I ended up reworking the last commit to remove the usage of the job, which turned out being tricky to reason about, in favor of bespoke logic. I've also reworked the tests to use a fake clock, to actually test the different timings for the grace period and backoff.

/test

Rebased onto main to hopefully fix the ci-ces-migrate failure.

/test

Marked for backport to v1.16, given that kvstoremesh is no longer beta, and this fix missed the branch window.