Allow setting ztunnel TERMINATION_GRACE_PERIOD via Helm chart

This allows Helm configuration of 2 preexisting ztunnel config knobs:

Allow terminationGracePeriodSeconds (standard Kube param, Kube/our current default is 30, this changes it to helm-defined value + 1) to be set for the ztunnel Pod. This is the duration Kube waits after sending a SIGTERM before sending a more agressive/destructive TERM sig.

Reuses that Helm-defined value to populate ztunnel's existing TERMINATION_GRACE_PERIOD env var, which is related, and is the duration ztunnel waits, after getting a SIGTERM, to finish draining its own proxied connections.

(I'm open to better ways to handle this than just adding one second - having 2 separate values seems potentially more confusing/error-prone - the important thing is that one value is always bigger than the other by a reasonable margin)

Actually, it's a little less surprising if this is inverted, so the raw value is used for Kube and the -1 val is used for ztunnel.

@bleggett: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name
Commit
Details
Required
Rerun command

integ-ambient_istio
6642c5d971ebdfe86d528f7f41666b4332951917
link
true
/test integ-ambient

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

/test integ-ambient

On Mon, May 8, 2023 at 12:33 PM Ben Leggett @.***>
wrote:

Is 30s the best default ? Why not 30 min or 5 sec ? If we are making it
configurable, we should have some guidance on what an admin should set it
to.
30 sec is the K8S default, if unspecified (AFAICT,
https://cloud.google.com/blog/products/containers-kubernetes/kubernetes-best-practices-terminating-with-grace
)

I'm not sure this PR has the same behavior as the default - wouldn't the
env variable set on ztunnel make it wait for that interval instead of
stopping on SIGTERM ?
I also don't think a lot of users ( including myself ) understand the
impact of the env variable and termination for a daemon set like ztunnel -
will a new ztunnel
be started in parallel for the 30 sec ? Will the pods on the node stop
receiving since ztunnel is removed from the EndpointSlice ? Even for
deployments the
termination is a tricky area.
IMO the safest upgrade for CNI and ztunnel is combined with the kubelet and
node upgrade and using a node cordon/drain. But now that we support
sessions,
a proper upgrade would wait for the max duration of a termination duration
for any pod on the node. Which may be tricky to implement, but as a
workaround
we can have a much larger value.
It's far more complicated than it seems - I don't know how much we can put
in the comments in values.yaml, but we need to at least mention those
issues.

I'm generally against defining a shadow API in helm - but this case seems
to be working as intended, it sets k8s install options.
Yeah it's an existing K8S config option we don't expose, and the ztunnel
config option is behaviorally linked to it, so for now this seems like a
reasonable compromise.
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1538932041, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2XLQEODVHT3U566GNLXFFDCFANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because your review was requested.Message ID:
@.***>

My point was that (unless I'm missing something) this change is not a
no-op. Setting the pod option is a no-op, but the env variable was not set
before and changes the behavior.
On Mon, May 8, 2023 at 4:41 PM Ben Leggett @.***> wrote:

I'm not sure this PR has the same behavior as the default - wouldn't the
env variable set on ztunnel make it wait for that interval instead of
stopping on SIGTERM ?

On pod stop/delete, Kube will issue a SIGTERM, and wait for
terminationGracePeriodSeconds before forcibly killing the pod.
terminationGracePeriodSeconds defaults to 30 seconds.
2.
On receiving SIGTERM, ztunnel will block as it terminates any
outstanding connections, for the duration of TERMINATION_GRACE_PERIOD

which is configurable (but without this PR not via Helm), and defaults to
5 seconds.

Ideally these two values have to be in lockstep, if the former is smaller
than the latter then kube interrupts ztunnel's cleanup, and if the latter
is a ztunnel startup config option but not configurable via Helm, then it's
useless.
I also don't think a lot of users ( including myself ) understand the
impact of the env variable and termination for a daemon set like ztunnel.
yes, that's part of the motivation for exposing the standard Kube grace
period config and implicitly/automatically configuring ztunnel's grace
period based on it.
will a new ztunnel be started in parallel for the 30 sec ?
Depends on other factors, how you configure rollout for the DS (what is
maxUnavailable set to), etc, etc. Not really affected by this value at
all.
Our current default ztunnel daemonset Helm config sets maxUnavailable: 0
which IIRC means old ztunnel pods on the node will not recieve the SIGTERM
until their replacements are healthy. By this point the CNI should/ought to
have already detected the new one and reconfigured redirection.
Will the pods on the node stop receiving since ztunnel is removed from the
EndpointSlice ? Even for deployments the termination is a tricky area. IMO
the safest upgrade for CNI and ztunnel is combined with the kubelet and
node upgrade and using a node cordon/drain. But now that we support
sessions, a proper upgrade would wait for the max duration of a termination
duration for any pod on the node. Which may be tricky to implement, but as
a workaround we can have a much larger value. It's far more complicated
than it seems - I don't know how much we can put in the comments in
values.yaml, but we need to at least mention those issues.
I agree that all those are issues, but they're somewhat orthogonal
to/require far more than letting people customize the K8S API
terminationGracePeriodSeconds field, and making sure ztunnel's related
field is always aligned with it.
It's a pretty standard field to expose via Helm chart, and the K8S default
here isn't changed from what you'd get if you didn't specify it. This PR
just synchronizes it with the correlated ztunnel value, so people don't set
terminationGracePeriodSeconds (a pretty standard k8s prop) and wonder why
ztunnel always terminates connections with a grace period of 5 seconds, no
matter what they set terminationGracePeriodSeconds to for the pod.
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1539043853, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2WV5F2EQBG5ZPATTTTXFGAAZANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because your review was requested.Message ID:
@.***>

My point was that (unless I'm missing something) this change is not a no-op. Setting the pod option is a no-op, but the env variable was not set before and changes the behavior.

Correct, the pod option is (by default) a no-op, the env var is not, as it changes the termination grace period to (podOption - 1 sec) - which means by default the ztunnel drain max wait period is 29 and not 5 seconds.
Do you have a better suggestion?
If we don't intend to expose TERMINATION_GRACE_PERIOD env var from ztunnel we should remove it.
If we intend to expose it we need to offer a mechanism to set it.
If we need to set it, it needs to be smaller than the related pod timeout, and it would be good to ensure that via Helm constraints.

/test bookinfo-build

@bleggett: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name
Commit
Details
Required
Rerun command

bookinfo-build_istio
6642c5d971ebdfe86d528f7f41666b4332951917
link
true
/test bookinfo-build

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

/test bookinfo-build

@costinm RE last comment - do you have an alternate/preferred suggestion here?
The ztunnel env var predates this PR, but is non-configurable without this PR.

That's another problem - given how tricky it is to install and restart
ztunnel - do we really want setting the drain interval to be an install
time option,
and adjusting it to require restarting ztunnel ? Including potential node
cordon for users who want to be safe, and draining (with the previous value
since the new value takes effect next time) ?
I understand some of the arguments on not creating a new Istiod helm chart
and cleaning up that mess - but they don't apply to ztunnel,
we don't have a legacy of users who would get confused, or CI/CDs that
somehow use helm as API.
My suggestion is to use ztunnel yaml config, as a regular mounted config
map that is controlled by user and not helm - and not repeat
the mistake of having the config of ztunnel as an install-only option with
changes requiring rollout, draining and all that.
This is the perfect example of why install options are a bad API - I can't
imagine a user who wants to adjust the drain duration will want to
do it by doing a drain with the old duration and going trough the entire
ztunnel restart process and risks so next time ztunnel restarts will
have the new drain setting. That's for the env variable - of course for the
pod setting we don't have options, but that is a maximum,
the env variable is really controlling how long ztunnel will wait.
On Thu, May 11, 2023 at 3:03 PM Ben Leggett @.***>
wrote:

@costinm https://github.com/costinm RE last comment - do you have an
alternate/preferred suggestion here?
The ztunnel env var predates this PR, but is non-configurable without this
PR.
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1544746618, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2UG2GMRHKR5L5X562LXFVO47ANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

@@.*** @.***> -  my last comment is far
broader than this PR.
Assuming ztunnel may be embedded in native CNI ( at least for the purpose
of upgrade - i.e. upgrade with the CNI/kubelet/node, as part of platform)
and not under control of regular CI/CD - I think we should use the same
pattern that kube-system has, where config maps like coredns or
calico are exposed and controllable by user. Or even better - define a
proto and CRD for ambient configs ( I suspect many are not
really specific to ztunnel implementation ).
If I remember correctly, one of the goals was to only make new mistakes -
and not repeat old ones...
On Fri, May 12, 2023 at 9:51 AM Costin Manolache @.***> wrote:

That's another problem - given how tricky it is to install and restart
ztunnel - do we really want setting the drain interval to be an install
time option,
and adjusting it to require restarting ztunnel ? Including potential node
cordon for users who want to be safe, and draining (with the previous value
since the new value takes effect next time) ?
I understand some of the arguments on not creating a new Istiod helm chart
and cleaning up that mess - but they don't apply to ztunnel,
we don't have a legacy of users who would get confused, or CI/CDs that
somehow use helm as API.
My suggestion is to use ztunnel yaml config, as a regular mounted config
map that is controlled by user and not helm - and not repeat
the mistake of having the config of ztunnel as an install-only option with
changes requiring rollout, draining and all that.
This is the perfect example of why install options are a bad API - I can't
imagine a user who wants to adjust the drain duration will want to
do it by doing a drain with the old duration and going trough the entire
ztunnel restart process and risks so next time ztunnel restarts will
have the new drain setting. That's for the env variable - of course for
the pod setting we don't have options, but that is a maximum,
the env variable is really controlling how long ztunnel will wait.
On Thu, May 11, 2023 at 3:03 PM Ben Leggett @.***>
wrote:

@costinm https://github.com/costinm RE last comment - do you have an
alternate/preferred suggestion here?
The ztunnel env var predates this PR, but is non-configurable without
this PR.
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1544746618, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2UG2GMRHKR5L5X562LXFVO47ANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

That's fair. Although the Pod level setting we have no choice really, but we could just always set that really high and have it handled in-app by ztunnel.
So you are proposing this is a dynamically-updated configmap read by ztunnel, yes?
Do you have a strong preference on that configmap being in or out of Helm?

I personally would prefer setting it via helm, i think nearly everyone can understand how to configure this via reading helm values.yaml file, not needing to read another config map doc (plus finding the doc in the first place).
IIUC, @bleggett 's PR enabled us to set the grace period to 30s for env var & pod, and without his PR the env var is 1s which is very short to drain any active connections.
I don't see a better way to do this, so I'd vote LGTM
I'm tagging a few other @istio/wg-environments-maintainers to chime in if you they want to.

That's another problem - given how tricky it is to install and restart ztunnel - do we really want setting the drain interval to be an install time option, and adjusting it to require restarting ztunnel ? Including potential node cordon for users who want to be safe, and draining (with the previous value since the new value takes effect next time) ? I understand some of the arguments on not creating a new Istiod helm chart and cleaning up that mess - but they don't apply to ztunnel, we don't have a legacy of users who would get confused, or CI/CDs that somehow use helm as API. My suggestion is to use ztunnel yaml config, as a regular mounted config map that is controlled by user and not helm - and not repeat the mistake of having the config of ztunnel as an install-only option with changes requiring rollout, draining and all that.

Totally fair, that's a much broader existing problem that ztunnel doesn't have a dynamic config mechanism. I agree it probably should have that in some form, and today it has nothing like that outside of xDS.
If we go the CM route, realistically that CM would still be populated/created by Helm initially, mounted into the ztunnel pod, and we would add dynamic config reloading toztunnel.
In which case, this PR still directly feeds into that - exposing terminationGracePeriodSeconds as a Helm option is still going to be something we would do there, we would just interpolate that into the initial ztunnel CM differently, and once ztunnel had the ability to detect and reload on CM changes, it could do what it wanted with the updated value.

If we go the CM route, realistically that CM would still be populated/created by Helm initially, mounted into the ztunnel pod, and we would add dynamic config reloading toztunnel.

I tend to agree, and I am not convinced if needs to be something dynamic plus the pod config for termination_grace_period  would require restart ztunnel pod anyway?

On Fri, May 12, 2023 at 11:17 AM John Howard @.***>
wrote:

That's fair. Although the Pod level setting we have no choice really, but
we could just always set that really high and have it handled in-app by
ztunnel.
So you are proposing this is a dynamically-updated configmap read by
ztunnel, yes?
Do you have a strong preference on that configmap being in or out of Helm?

I have a very strong preference to have it out of helm - and loaded using
normal config map mount, not watcher.
If ztunnel becomes part of an 'upstream' or platform - users will have no
control over installing/reinstalling ztunnel,
but they can modify the config map - like CoreDNS and quite a few other
platform components in kube-system.
It is probably also fine to push it from XDS - like we do for ProxyConfig -
but I think that would be overkill, overcomplex
and not really needed.

Message ID: @.***>

On Fri, May 12, 2023 at 11:24 AM Lin Sun @.***> wrote:

I personally would prefer setting it via helm, i think nearly everyone can
understand how to configure this via reading helm values.yaml file, not
needing to read another config map doc (plus finding the doc in the first
place).

That assumes ztunnel will remain a user-managed component, and user will
understand how to safely upgrade it, and
that users prefer dangerous and disruptive restarts of the per-node ztunnel
for the smallest fine tunning.
CRDs and config maps are used broadly in K8S - in particular for things
that don't require reinstalling, as changing this particular config is.
Maybe for Istiod the argument is true and some people prefer doing
revision-based upgrades from CI/CD ( with cluster-wide permissions
as Istio upgrades require ) - but regardless of preferences I hope we agree
it is not a best practice from security, operations, config validations,
etc. At least for ztunnel.

IIUC, @bleggett https://github.com/bleggett 's PR enabled us to set the
grace period to 30s for env var & pod, and without his PR the env var is 1s
which is very short to drain any active connections.
I don't see a better way to do this, so I'd vote LGTM

I mean - having an optional config map that include this is clearly a
better way as it applies on the current restart, not the next one,
and doesn't require restarting ztunnel to specify how long it should wait
on restart. How would you explain to the user - it's a change
in the future ?
Message ID: @.***>

CRDs and config maps are used broadly in K8S - in particular for things that don't require reinstalling, as changing this particular config is.

I agree with using CM and mounting it. Not sure why it can't be in the helm chart though.
If its part of the platform then users just don't use helm. But if they are using helm, they should be able to use helm I would think.

On Fri, May 12, 2023 at 11:41 AM Ben Leggett @.***>
wrote:

That's another problem - given how tricky it is to install and restart
ztunnel - do we really want setting the drain interval to be an install
time option, and adjusting it to require restarting ztunnel ? Including
potential node cordon for users who want to be safe, and draining (with the
previous value since the new value takes effect next time) ? I understand
some of the arguments on not creating a new Istiod helm chart and cleaning
up that mess - but they don't apply to ztunnel, we don't have a legacy of
users who would get confused, or CI/CDs that somehow use helm as API. My
suggestion is to use ztunnel yaml config, as a regular mounted config map
that is controlled by user and not helm - and not repeat the mistake of
having the config of ztunnel as an install-only option with changes
requiring rollout, draining and all that.
Totally fair, that's a much broader existing problem that ztunnel doesn't
have a dynamic config mechanism. I agree it probably should have that in
some form, and today it has nothing like that outside of xDS.

It has a yaml config - I think it can periodically read it or watch for
file changes, rust has libraries for this. We don't need to make it super
complex.

If we go the CM route, realistically that CM would still be
populated/created by Helm initially, mounted into the ztunnel pod, and we
would add dynamic config reloading toztunnel.

Maybe it can be populated by Helm initially - but I think it is critical to
behave like core-dns or Calico or all other system config maps, in the sense
that once a user edits the config map - it will not be replaced by the next
upgrade.
Configmaps are intended to represent K8S resources that are almost always
user modifiable - mesh config is the only exception I know
where user configs last until the next helm upgrade.

In which case, this PR still directly feeds into that - exposing
terminationGracePeriodSeconds as a Helm option is still going to be
something we would do there, we would just interpolate that into the
initial ztunnel CM differently, and once ztunnel had the ability to
detect and reload on CM changes, it could do what it wanted with the
updated value

I have no concerns with the pod option - just the env variable.
I think we can set the pod option to a very high value ( 3 h ) - so users
don't need to worry, it is a max not the actual drain duration.

On Fri, May 12, 2023 at 2:23 PM John Howard @.***>
wrote:

CRDs and config maps are used broadly in K8S - in particular for things
that don't require reinstalling, as changing this particular config is.
I agree with using CM and mounting it. Not sure why it can't be in the
helm chart though.
If its part of the platform then users just don't use helm. But if they
are using helm, they should be able to use helm I would think.

Consistency with other k8s platform settings ( calico, core-dns, etc) ?
I don't mind if helm creates it - the fundamental problem is that user
changes to the config map should not be replaced on the next upgrade.
And if we expect ztunnel to become 'platform' - managed along with the
node, kubelet and CNI - we do need to provide a way for
users to configure things independent of the platform admin.
I don't know if platform admins ( GKE, AWS, etc ) use helm or not for
configuring kubelet, CNI and other system configs, but it is clear
that other system components like core-dns allow some user control by
making changes to the configmap.
Message ID: @.***>

Also the argument that if they use helm - they should be able to configure
everything using values.yaml may make sense
for Istiod ( because that's what we've traditionally done and set as
expectation ), but it is clearly not the case for
all k8s components - many do use CRDs or user-modifiable config maps as
well. Istiod included - many of our
options ( Secrets, initial CA when intermediary CA is used, etc) are still
independent of helm.
On Fri, May 12, 2023 at 2:29 PM Costin Manolache @.***> wrote:

On Fri, May 12, 2023 at 2:23 PM John Howard @.***>
wrote:

CRDs and config maps are used broadly in K8S - in particular for things
that don't require reinstalling, as changing this particular config is.
I agree with using CM and mounting it. Not sure why it can't be in the
helm chart though.
If its part of the platform then users just don't use helm. But if they
are using helm, they should be able to use helm I would think.

Consistency with other k8s platform settings ( calico, core-dns, etc) ?
I don't mind if helm creates it - the fundamental problem is that user
changes to the config map should not be replaced on the next upgrade.
And if we expect ztunnel to become 'platform' - managed along with the
node, kubelet and CNI - we do need to provide a way for
users to configure things independent of the platform admin.
I don't know if platform admins ( GKE, AWS, etc ) use helm or not for
configuring kubelet, CNI and other system configs, but it is clear
that other system components like core-dns allow some user control by
making changes to the configmap.
Message ID: @.***>

I have no concerns with the pod option - just the env variable.

I get that - but the latter needs to always be smaller than the former - they are implicitly linked.

I think we can set the pod option to a very high value ( 3 h ) - so users
don't need to worry, it is a max not the actual drain duration.

That leads to odd behavior if ztunnel hangs on termination, and defeats the purpose of the K8S API - my point is we shouldn't have opinions on what terminationGracePeriodSeconds is set to - it's a K8S lifecycle setting and out of our hands.
ztunnel has to respond to whatever that setting is, and make sure it's termination grace is smaller - it can't do anything else.

to get some real data instead of preferences of developers on our team, I researched the top 10 Helm charts and whether they put configmaps in the chart or have users configure them externally to Helm. I also included Calico and Cilium, since they are similar to ztunnel (but not top 10):
configmaps in helm:
kube-prometheus-stack
ingress-nginx
cert-manager
argocd
prometheus
redis
grafana
kubernetes-dashboard
postgresql
Cilium (not top 10, but included as a CNI)
Calico (not top 10, but included as a CNI)
configmaps not in helm:
traefik - but there is no configmaps used at all in the software

tl;dr I have not found any helm charts that have users configure configmaps to configure the software outside of helm.

On Fri, May 12, 2023 at 2:33 PM Ben Leggett @.***>
wrote:

I have no concerns with the pod option - just the env variable.
I get that - but the latter needs to always be smaller than the former -
they are implicitly linked.
I think we can set the pod option to a very high value ( 3 h ) - so users
don't need to worry, it is a max not the actual drain duration.
That leads to odd behavior if ztunnel hangs on termination, and defeats
the purpose of the K8S API - my point is we shouldn't have opinions on
what terminationGracePeriodSeconds is set to - it's a K8S lifecycle
setting and out of our hands.
ztunnel has to respond to whatever that setting is, and make sure it's
termination grace is smaller - it can't do anything else.

I don't know if we can use a projection - or if such a corner case needs
much more than documentation.
But it doesn't need to be smaller - if it is larger the k8s pod setting
takes priority and terminates earlier.
Message ID: @.***>

But it doesn't need to be smaller - if it is larger the k8s pod setting
takes priority and terminates earlier.

Then let's just remove TERMINATION_GRACE_PERIOD from ztunnel entirely, and let K8S the process manager.
Just rely only on terminationGracePeriodSeconds

Ok, how about: ztunnel install ( and maybe istiod ) should NOT depend or
require helm.
That means vendors who manage the components should not be forced to use
helm. Since Istio supports 3 different install methods -
2 of which may internally as implementation use helm, but expose their own
yaml config and one as a CRD - I think
we have a precedent for this in istio.
I believe most of the examples you gave also provide operators - and the
operators have CRDs that control the
options of the component.
If Calico does create and overrides the configmap using helm - we are in
huge problems, because our instructions
for configuring calico include modifying the configmap. Does it mean the
configs are until next time user does a helm
upgrade on calico ?
On Fri, May 12, 2023 at 2:34 PM John Howard @.***>
wrote:

to get some real data instead of preferences of developers on our team, I
researched the top 10 Helm charts and whether they put configmaps in the
chart or have users configure them externally to Helm. I also included
Calico and Cilium, since they are similar to ztunnel (but not top 10):
configmaps in helm:
kube-prometheus-stack
ingress-nginx
cert-manager
argocd
prometheus
redis
grafana
kubernetes-dashboard
postgresql
Cilium (not top 10, but included as a CNI)
Calico (not top 10, but included as a CNI)
configmaps not in helm:
traefik - but there is no configmaps used at all in the software
tl;dr I have not found any helm charts that have users configure
configmaps to configure the software outside of helm.
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1546330428, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2WIOJQBAOUELFTMT73XF2UH3ANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

On Fri, May 12, 2023 at 2:38 PM Ben Leggett @.***>
wrote:

But it doesn't need to be smaller - if it is larger the k8s pod setting
takes priority and terminates earlier.
Then let's just remove TERMINATION_GRACE_PERIOD from ztunnel entirely,
and let K8S the process manager.
Just rely only on terminationGracePeriodSeconds

I think this may work - ztunnel still gets the signal and can stop
accepting new connections, maybe send GOAWAY
on existing connections - I don't think the env variable does anything
special. If active connections is 0 - it can exit.

—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1546332974, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2USBCM2DYL5BB262WLXF2UXHANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

If Calico does create and overrides the configmap using helm - we are in huge problems, because our instructions for configuring calico include modifying the configmap. Does it mean the configs are until next time user does a helm upgrade on calico ?

I don't think that instruction is a serious doc - its a hack to get it working - not suitable for real world.

On Fri, May 12, 2023 at 2:45 PM John Howard @.***>
wrote:

If Calico does create and overrides the configmap using helm - we are in
huge problems, because our instructions for configuring calico include
modifying the configmap. Does it mean the configs are until next time user
does a helm upgrade on calico ?
I don't think that instruction is a serious doc - its a hack to get it
working - not suitable for real world.

Does it mean Calico is not supported either ? Just Kind, or what do we
really support ?
At least on GKE Calico seems to be installed by addonmanager, and the CRD
we are changing doesn't appear to be created by helm
or addonmanager - I don't know how it was originally created, but as a
proper CRD with "v1" I would expect it reflects a supported
API surface, not a hack.

Message ID: @.***>

On Fri, May 12, 2023 at 2:38 PM Ben Leggett @.***> wrote: But it doesn't need to be smaller - if it is larger the k8s pod setting takes priority and terminates earlier. Then let's just remove TERMINATION_GRACE_PERIOD from ztunnel entirely, and let K8S the process manager. Just rely only on terminationGracePeriodSeconds
I think this may work - ztunnel still gets the signal and can stop accepting new connections, maybe send GOAWAY on existing connections - I don't think the env variable does anything special. If active connections is 0 - it can exit.

Yep. ztunnel pod will get SIGTERM first from K8S, it can try to drain connections for $UNKNOWN_AMOUNT_OF_TIME (e.g. whatever K8S waits as directed by terminationGracePeriodSeconds, and if it doesn't clean everything up by the time K8S checks in again and SIGKILL's or whatever, then too bad.
Totally OK removing the ZT flag and relying on normal platform lifecycle management, I just don't want to keep both and only configure one.

Sgtm.
That resolves this PR ( k8s pod setting clearly belong in helm ), but we still need separately to discuss if we mandate helm as the only API and config surface, and how the UX will look like given the risks and costs of reinstalling for simple config changes.

Sgtm.
That resolves this PR ( k8s pod setting clearly belong in helm ), but we still need separately to discuss if we mandate helm as the only API and config surface, and how the UX will look like given the risks and costs of reinstalling for simple config changes.

Agree, sounds good.

I'm moving this discuss over ztunnel's TERMINATION_GRACE_PERIOD  to a ztunnel repo issue, but one thing that might be a sticking point for removing TERMINATION_GRACE_PERIOD from ztunnel proper is it hampers the "outside of K8S" story for ztunnel.

If we go with the K8S setting and remove the ztunnel setting ("outside-in" - then ztunnel running outside of K8S would need an external watchdog to do SIGTERM-wait-SIGKILL, which seems bad.

Or leave TERMINATION_GRACE_PERIOD for 'outside of k8s' - but I'm not
entirely sure I understand how termination works on a VM.
My understanding was that systemd ( or docker if ztunnel runs in a
container ) will also send SIGTERM first. And eventually SIGKILL.
Any draining or termination needs to be initiated by the VM/docker somehow.
If we ( or user ) automates ztunnel upgrade - they will need some tool to
send SIGTERM and SIGKILL, how else can they drain ?
On Mon, May 15, 2023 at 7:14 AM Ben Leggett @.***>
wrote:

Sgtm.
That resolves this PR ( k8s pod setting clearly belong in helm ), but we
still need separately to discuss if we mandate helm as the only API and
config surface, and how the UX will look like given the risks and costs of
reinstalling for simple config changes.

Agree, sounds good.
2.
I'm moving this discuss over ztunnel's TERMINATION_GRACE_PERIOD to a
ztunnel repo issue, but one thing that might be a sticking point for
removing TERMINATION_GRACE_PERIOD from ztunnel proper is it hampers
the "outside of K8S" story for ztunnel.

If we go with the K8S setting and remove the ztunnel setting
("outside-in" - then ztunnel running outside of K8S would need an external
watchdog to do SIGTERM-wait-SIGKILL, which seems bad.

—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1547953730, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2XSOMUUM626MHEUICTXGI24VANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

If we ( or user ) automates ztunnel upgrade - they will need some tool to send SIGTERM and SIGKILL, how else can they drain ?

I'm fine with making that an explicit requirement, and yeah totally that's what e.g. systemd does - just making sure everyone is OK with that.

Or leave TERMINATION_GRACE_PERIOD for 'outside of k8s' - but I'm not entirely sure I understand how termination works on a VM. My understanding was that systemd ( or docker if ztunnel runs in a container ) will also send SIGTERM first. And eventually SIGKILL. Any draining or termination needs to be initiated by the VM/docker somehow. If we ( or user ) automates ztunnel upgrade - they will need some tool to send SIGTERM and SIGKILL, how else can they drain ?

Aight so as per the discuss in https://github.com/istio/ztunnel/issues/519 I think we're at the following point.

We can't remove either setting (k8s or ztunnel)- we need both, so ztunnel can have guarantees about the window it has to do graceful RST.
We need to set the ztunnel property to LESS-THAN the k8s setting, to support 1)
That means what's in this PR is still needed:

We need to set the k8s property
Helm must be used to set the k8s property
We need to set the ztunnel property to LESS-THAN the k8s property

@costinm brings up the very good point that the ztunnel value in question here should (along with others) not simply be an install-time-only env variable, as they are today.

@costinm are you ok with approving this given the above, and I will create a separate istio/ztunnel issue to track the need for dynamic config?

I commented on the ztunnel bug as well.
I think we're again moving into NIH territory - inventing a new API ( or 2
when you implement the dynamic one ) without
clear requirements and design - and adding a pretty big operating burden on
our users who will need to understand how
this is supposed to be used.
What is the expected stop process for ztunnel and why ? AFAIK, the only
reasons ztunnel would be stopped is:

users want to stop using ambient - but this process starts with removing
the annotation on namespaces, restarting
pods or whatever other processes get them to stop using istio. In this case
it doesn't really matter how ztunnel dies.
upgrade of ztunnel. I assume that's the case we want to optimize for, to
avoid data loss.

In the upgrade case, so far the options I know are cordoning the node - or
using the CNI layer to move all new connections
to the new upgraded ztunnel instance. For a VM - similar mechanism exist (
start the new process or container first, use
low level networking to redirect new stuff to the new process).
The only 'actionable' thing the old ztunnel can do in response to SIGTERM
is to send GOAWAY and stop accepting -
both will accelerate the move of connections to the new ztunnel. For any
in-progress TCP connection, including long
lived streams - there is nothing ztunnel can do using the env variable.
SIGKILL will send the RST and terminate the
connection non-gracefully, and I don't think there is any other graceful
method ( the GOAWAY on the H2 connection
may signal some smart clients to start gracefully terminating long-lived
connections ).
So going back to this - what do we tell operators to do with this helm (
and future dynamic ) API ? Are we committed to
support this as an API long term - and test and provide this ( TBD )
behavior as part of the ambient contract ? If
ambient gets implemented 'natively' in CNI or network layers - would we
expect them to also support such an API ?
On Tue, May 16, 2023 at 7:25 AM Ben Leggett @.***>
wrote:

Or leave TERMINATION_GRACE_PERIOD for 'outside of k8s' - but I'm not
entirely sure I understand how termination works on a VM. My understanding
was that systemd ( or docker if ztunnel runs in a container ) will also
send SIGTERM first. And eventually SIGKILL. Any draining or termination
needs to be initiated by the VM/docker somehow. If we ( or user ) automates
ztunnel upgrade - they will need some tool to send SIGTERM and SIGKILL, how
else can they drain ?
Aight so as per the discuss in istio/ztunnel#519
https://github.com/istio/ztunnel/issues/519 I think we're at the
following point.

We can't remove either setting (k8s or ztunnel)- we need both, so
ztunnel can have guarantees about the window it has to do graceful RST.
We need to set the ztunnel property to LESS-THAN the k8s setting,
to support 1)
That means what's in this PR is still needed:

We need to set the k8s property
Helm must be used to set the k8s property
We need to set the ztunnel property to LESS-THAN the k8s property

@costinm https://github.com/costinm brings up the very good point
that the ztunnel value in question here should (along with others)
not simply be an install-time-only env variable, as they are today.

@costinm https://github.com/costinm are you ok with approving this
given the above, and I will create a separate istio/ztunnel issue to
track the need for dynamic config?
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1549781374, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2TRUSRIT2M7H4IZ5HLXGOE33ANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

BTW - if you really have a need for this PR, happy to approve it, I don't
want to block you - but I don't think the
helm install of ztunnel is part of the 'contract' or 'standard' ambient API
surface ( just like I don't think all helm
options for istio are anything but a helm install API ). I am not trying to
block this PR - the bar for helm
settings and env variables is pretty low and I don't remember many cases
where such changes were blocked.
I do not think we should make it a dynamic API - my point ( which I
probably failed to explain ) was that if we
have a clear use case and expect operators to need to control it, it would
be best if it is a proper CRD or
configmap that operators can actually use independent of how they installed
ztunnel ( or whatever implements
the ambient protocols ).
On Tue, May 16, 2023 at 8:00 AM Costin Manolache @.***> wrote:

I commented on the ztunnel bug as well.
I think we're again moving into NIH territory - inventing a new API ( or 2
when you implement the dynamic one ) without
clear requirements and design - and adding a pretty big operating burden
on our users who will need to understand how
this is supposed to be used.
What is the expected stop process for ztunnel and why ? AFAIK, the only
reasons ztunnel would be stopped is:

users want to stop using ambient - but this process starts with removing
the annotation on namespaces, restarting
pods or whatever other processes get them to stop using istio. In this
case - it doesn't really matter how ztunnel dies.
upgrade of ztunnel. I assume that's the case we want to optimize for, to
avoid data loss.

In the upgrade case, so far the options I know are cordoning the node - or
using the CNI layer to move all new connections
to the new upgraded ztunnel instance. For a VM - similar mechanism exist (
start the new process or container first, use
low level networking to redirect new stuff to the new process).
The only 'actionable' thing the old ztunnel can do in response to SIGTERM
is to send GOAWAY and stop accepting -
both will accelerate the move of connections to the new ztunnel. For any
in-progress TCP connection, including long
lived streams - there is nothing ztunnel can do using the env variable.
SIGKILL will send the RST and terminate the
connection non-gracefully, and I don't think there is any other graceful
method ( the GOAWAY on the H2 connection
may signal some smart clients to start gracefully terminating long-lived
connections ).
So going back to this - what do we tell operators to do with this helm (
and future dynamic ) API ? Are we committed to
support this as an API long term - and test and provide this ( TBD )
behavior as part of the ambient contract ? If
ambient gets implemented 'natively' in CNI or network layers - would we
expect them to also support such an API ?
On Tue, May 16, 2023 at 7:25 AM Ben Leggett @.***>
wrote:

Or leave TERMINATION_GRACE_PERIOD for 'outside of k8s' - but I'm not
entirely sure I understand how termination works on a VM. My understanding
was that systemd ( or docker if ztunnel runs in a container ) will also
send SIGTERM first. And eventually SIGKILL. Any draining or termination
needs to be initiated by the VM/docker somehow. If we ( or user ) automates
ztunnel upgrade - they will need some tool to send SIGTERM and SIGKILL, how
else can they drain ?
Aight so as per the discuss in istio/ztunnel#519
https://github.com/istio/ztunnel/issues/519 I think we're at the
following point.

We can't remove either setting (k8s or ztunnel)- we need both, so
ztunnel can have guarantees about the window it has to do graceful RST.
We need to set the ztunnel property to LESS-THAN the k8s setting,
to support 1)
That means what's in this PR is still needed:

We need to set the k8s property
Helm must be used to set the k8s property
We need to set the ztunnel property to LESS-THAN the k8s property

@costinm https://github.com/costinm brings up the very good
point that the ztunnel value in question here should (along with others)
not simply be an install-time-only env variable, as they are today.

@costinm https://github.com/costinm are you ok with approving this
given the above, and I will create a separate istio/ztunnel issue to
track the need for dynamic config?
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1549781374, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2TRUSRIT2M7H4IZ5HLXGOE33ANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

I missed on case for ztunnel termination - when operating as a sidecar, the
termination process needs to be different
and AFAIK it does need the setting ( which is already defined for sidecars
):

on SIGTERM - continue to accept requests ( persistent sessions, app may
do its own graceful shutdown)
after the drain duration - which for sidecars can be set per pod - start
GOAWAY and rejecting new connections.
if active connections get to zero - exit.
SIGKILL terminates both app and sends RST on remaining connections
( not sure if I got this right - and the quick search didn't find a good
page with how we shutdown envoy, we should probably
add more tests and docs around this )

On Tue, May 16, 2023 at 8:06 AM Costin Manolache @.***> wrote:

BTW - if you really have a need for this PR, happy to approve it, I don't
want to block you - but I don't think the
helm install of ztunnel is part of the 'contract' or 'standard' ambient
API surface ( just like I don't think all helm
options for istio are anything but a helm install API ). I am not trying
to block this PR - the bar for helm
settings and env variables is pretty low and I don't remember many cases
where such changes were blocked.
I do not think we should make it a dynamic API - my point ( which I
probably failed to explain ) was that if we
have a clear use case and expect operators to need to control it, it would
be best if it is a proper CRD or
configmap that operators can actually use independent of how they
installed ztunnel ( or whatever implements
the ambient protocols ).
On Tue, May 16, 2023 at 8:00 AM Costin Manolache @.***> wrote:

I commented on the ztunnel bug as well.
I think we're again moving into NIH territory - inventing a new API ( or
2 when you implement the dynamic one ) without
clear requirements and design - and adding a pretty big operating burden
on our users who will need to understand how
this is supposed to be used.
What is the expected stop process for ztunnel and why ? AFAIK, the only
reasons ztunnel would be stopped is:

users want to stop using ambient - but this process starts with
removing the annotation on namespaces, restarting
pods or whatever other processes get them to stop using istio. In this
case - it doesn't really matter how ztunnel dies.
upgrade of ztunnel. I assume that's the case we want to optimize for,
to avoid data loss.

In the upgrade case, so far the options I know are cordoning the node -
or using the CNI layer to move all new connections
to the new upgraded ztunnel instance. For a VM - similar mechanism exist
( start the new process or container first, use
low level networking to redirect new stuff to the new process).
The only 'actionable' thing the old ztunnel can do in response to SIGTERM
is to send GOAWAY and stop accepting -
both will accelerate the move of connections to the new ztunnel. For any
in-progress TCP connection, including long
lived streams - there is nothing ztunnel can do using the env variable.
SIGKILL will send the RST and terminate the
connection non-gracefully, and I don't think there is any other graceful
method ( the GOAWAY on the H2 connection
may signal some smart clients to start gracefully terminating long-lived
connections ).
So going back to this - what do we tell operators to do with this helm (
and future dynamic ) API ? Are we committed to
support this as an API long term - and test and provide this ( TBD )
behavior as part of the ambient contract ? If
ambient gets implemented 'natively' in CNI or network layers - would we
expect them to also support such an API ?
On Tue, May 16, 2023 at 7:25 AM Ben Leggett @.***>
wrote:

Or leave TERMINATION_GRACE_PERIOD for 'outside of k8s' - but I'm not
entirely sure I understand how termination works on a VM. My understanding
was that systemd ( or docker if ztunnel runs in a container ) will also
send SIGTERM first. And eventually SIGKILL. Any draining or termination
needs to be initiated by the VM/docker somehow. If we ( or user ) automates
ztunnel upgrade - they will need some tool to send SIGTERM and SIGKILL, how
else can they drain ?
Aight so as per the discuss in istio/ztunnel#519
https://github.com/istio/ztunnel/issues/519 I think we're at the
following point.

We can't remove either setting (k8s or ztunnel)- we need both, so
ztunnel can have guarantees about the window it has to do graceful RST.
We need to set the ztunnel property to LESS-THAN the k8s setting,
to support 1)
That means what's in this PR is still needed:

We need to set the k8s property
Helm must be used to set the k8s property
We need to set the ztunnel property to LESS-THAN the k8s
property

@costinm https://github.com/costinm brings up the very good
point that the ztunnel value in question here should (along with others)
not simply be an install-time-only env variable, as they are today.

@costinm https://github.com/costinm are you ok with approving this
given the above, and I will create a separate istio/ztunnel issue to
track the need for dynamic config?
—
Reply to this email directly, view it on GitHub
https://github.com/istio/istio/pull/44813#issuecomment-1549781374, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAUR2TRUSRIT2M7H4IZ5HLXGOE33ANCNFSM6AAAAAAX2J2BZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

BTW - if you really have a need for this PR, happy to approve it, I don't want to block you - but I don't think the helm install of ztunnel is part of the 'contract' or 'standard' ambient API surface ( just like I don't think all helm options for istio are anything but a helm install API ).

Yeah, I don't think we can legitimately avoid letting people set the K8S prop exposed in this PR. And if we set the K8S prop, under the hood the ztunnel prop needs to be LESS-THAN the k8s prop.
So we do need the outward-facing change here. The internal configuration of the ztunnel prop we can redo or drop later, but for now it does need to track the k8s value.
Before this PR you couldn't set the ztunnel config prop directly via Helm. After this PR you can't set the ztunnel config prop directly via Helm.
Since the only outward-facing thing we are explicitly allowing people to set/expose directly is the K8S property here, I don't think we're painting ourselves into a corner here.

Aight - @costinm @howardjohn I've updated this PR to just set the K8S prop, and created https://github.com/istio/ztunnel/pull/522 to remove the otherwise-dependent ztunnel setting.

@costinm @Monkeyanator @jacob-delgado
Are we good to approve/merge this?
This is now a very simple PR that simply exposes a standard k8s Pod API prop to the chart.


Unable to add records on first run

Application is not properly stored on first run.
Work around: Change scope and add record again.

#169


Support for heterogeneous collections?

I am implementing a search endpoint, where the search may return different sorts of resources, as in this scenario. I don't think this is currently supported, is it?
Do you think this library should heterogenous collections , given that json:api supports them?
I'm not sure yet what would be the best approach to implementing, although I'd be happy to try a few things and see what works.
In the meantime can require multiple requests to perform a search as a workaround.
Cheers!

Can you link me to the specification where it describes that? I saw that you can post up non-heterogeneous data. I wish they provided a test suite :P

I wish I could link to where it describes that in the spec 😬 but it is not explicitly stated. It follows from the description in [document structure(http://jsonapi.org/format/#document-structure). The only restriction on a "primary data" field is that it is either a resource object or an array of resource objects, where each resource object specifies its own type. Notably, I don't think the spec says anything about resource objects having to be the same type.
Some other mentions

cited as a benefit of json:api in a hacker news comment by one of the spec authors
mentioned in a github issue

I agree, a test suite would be nice. Or even a few more examples.

Yea I think we could support it, I'm uncertain how specifically but I am
open to ideas/PR's.
On Thu, Jun 14, 2018 at 10:56 AM, Eric Watson<EMAIL_ADDRESS>wrote:

I wish I could link to where it describes that in the spec 😬 but it is
not explicitly stated. It follows from the description in [document
structure(http://jsonapi.org/format/#document-structure). The only
restriction on a "primary data" field is that it is either a resource
object or an array of resource objects, where each resource object
specifies its own type. Notably, I don't think the spec says anything about
resource objects having to be the same type.
Some other mentions

cited as a benefit of json:api in a hacker news comment
https://news.ycombinator.com/item?id=9281102 by one of the spec
authors
mentioned in a github issue
https://github.com/json-api/json-api/issues/278

I agree, a test suite would be nice. Or even a few more examples.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/jeregrine/jsonapi/issues/118#issuecomment-397346456,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGKFnOoen-4R7eFMjQ6IwG8zYqNdSujks5t8oesgaJpZM4Un2jm
.

The github references heterogenius linked collections which we support.
I think maybe the way you could solve this is to have a new resource called SearchResult which has linked resources that represent the value. JSONAPI resources/view need not all be writeable or have models in the DB.

Cool—I'll look into linked resources and see where that takes me. Thanks!

Yea sorry you'd have a Search Result, it would have linked resources that
are inncluded.
On Thu, Jun 14, 2018 at 12:00 PM, Eric Watson<EMAIL_ADDRESS>wrote:

@jeregrine https://github.com/jeregrine for clarity when you say
"linked collections", are you taking about the top-level
http://jsonapi.org/format/#document-top-level included collection?
(looks like maybe that key was changed at some point)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jeregrine/jsonapi/issues/118#issuecomment-397366568,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGKFtDcB_SkBmCyA0doCxGqY8CCHvJfks5t8paegaJpZM4Un2jm
.

Thanks for the push toward linked collections. I ended up making an endpoint GET /search/:id, where id is the url-encoded version of the search query. Responses have a has_many relationship for each resource type, and the resource objects (of various types) can be included in the top-level included field.
Closing this for now. Thanks again for the help!

Perfect!
On Thu, Jun 14, 2018 at 4:57 PM, Eric Watson<EMAIL_ADDRESS>wrote:

Closed #118 https://github.com/jeregrine/jsonapi/issues/118.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jeregrine/jsonapi/issues/118#event-1682249690, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGKFurFE_8suSVZzM0aHJvCf6tCqvT-ks5t8tw8gaJpZM4Un2jm
.


Dendron preview won't update when using selection extract to create a note

Please select the area your bug applies to. (Multiple selections are Possible. You can leave blank if you're not sure.)

[ ] Workspace.  VSCode workspace, vaults, Intellisense/autocomplete, Dendron settings
[ ] Lookup.  Dendron's Lookup Command
[x] Views.  Dendron Preview, Tree View, Side Panels in the UI
[ ] Schema.  Dendron Schemas
[ ] Pod.  Data import from / export to Dendron
[ ] Publish.  External Site Publish
[ ] Markdown.  Markdown features, such as syntax support and features

Describe the bug
Dendron preview won't update when using selection extract to create a note.
To Reproduce
Steps to reproduce the behavior:

create a note A with content
open preview
select and extract note A content to note B
preview not update

Expected behavior
Preview updates when a new created
Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information)

OS: [e.g. iOS]
Browser (if with published websites; include version): [e.g. chrome, safari]
Versions (all that apply to bug):

Dendron Extension: 0.107.2
VS Code version: 1.70.1

Additional context
Add any other context about the problem here.
Dendron Log file
Please attach the output of > Dendron:Dev: Open Logs here
dendron.log

Thank you for the report.
I can't reproduce this on my end, but we'll investigate what's happening and let you know when it gets resolved.
@namjul could this be related to the preview lock feature?

Sorry for my oversight, I didn't reproduce it with only dendron enabled.
I tried it again and successfully reproduced it with dendron and Prettier enabled.
Any further information should I provide to resolve this problem?

@HenryC-3 that's good. I will look into it.

@HenryC-3 we could replicate the issue when we changed set the vscode Setting "Auto Save" to onFocusChange.
It makes sense because when the selected text gets extracted from the note and vscode saves it more or less at the same time then the Preview will get notified of that change.
Can you confirm on your end by turning "Auto Save" off, to make sure that this is also the cause on your end.

@namjul Thanks for your effort.
This issue persists on my end. I tried it by turning autosave off, the preview won't update until I click the preview and then click in editor.

Here's the log file
dendron.log

Can you try to reload vscode after you turned of autosafe and then check again. It was necessary on my end.

@namjul Hi checked again and can't reproduce this time. But I did reload and then reproduced yesterday like commented above.
Maybe we should put this issue on hold for a while and see if others will encounter this problem as well.
Thank you for taking the time to investigate this issue.

I am also facing the same issue. Anyone has any solution.


Crash on multiple left side controller menu

SideMenuManager.menuLeftNavigationController  = Table 1 .. Dashboard 1
SideMenuManager.menuLeftNavigationController  = Table 2 .. Dashboard 2
Crash..
I have used following to avoid above error!
INSTEAD OF using this -
presentViewController(SideMenuManager.menuLeftNavigationController!, animated: true, completion: nil)
I have used this -
performSegueWithIdentifier("segue_menu_controller", sender: self)

Thanks for finding this issue, @rosevinod.
I'm guessing that you were calling presentViewController from viewWillAppear or viewDidAppear, which were being called twice. This was most likely due to adding/removing the viewControllers to the transition context.
I've checked in a fix in 0.1.6.


Add MapEntities trait for Transform

Objective

Since the refactor of the MapEntities trait to be more flexible (https://github.com/bevyengine/bevy/pull/11428), I wanted to switch my library to use bevy's MapEntities trait instead of an internal trait (also because it's the only way to mutate the inner entity of the Parent and Children components, bevy doesn't expose a public way to modify them)

In my library every message I send implements MapEntities, meaning that the message/component needs to potentially call map_entities() to map any entities inside the message/component from the remote world to the local world.

The problem is that I cannot directly use components that are defined in external crates (for example Transform, defined in bevy), because they don't implement MapEntities

Solution

Make bevy components implement a no-op MapEntities to indicate that they do not perform any entity mapping.

Alternatives

I guess the alternative would be to have some form of specialization? and to only call MapEntities if the message/component implements it

I really don't love adding yet another layer of "mandatory thing that must be added for every component type".
I'd rather see this information go into the Component trait if needed, or find another approach completely.

Agree with @alice-i-cecile here.
In bevy_replicon we just require registering mapped components separately.

Yes I also don't think this PR is a good solution; I just opened it so that we could start a discussion about potential options or alternatives.
Ah I see that in replicon you provide two separate registration functions (https://github.com/projectharmonia/bevy_replicon/blob/master/src/replicon_core/replication_rules.rs#L21) for 'non-mapped components' and 'mapped components', interesting


Needs to add instructions to clone the repo before Helm install

In the following document: https://github.com/oam-dev/rudr/blob/master/docs/setup/install.md, the first instruction should be for users to clone the repo :)

I can pick this up

I think this issue could be closed


Cannot determine the number of lines in C# repository

I'm having an issue when running code QL on my C# code with GitHub Actions. It analyzes the code fine, but it says in the logs that it can't count the total number of lines. It's not really affecting anything, but it suggested to drop an issue here.
Here's the relevant part of the log when running the action:
Running queries for csharp
  /opt/hostedtoolcache/CodeQL/0.0.0-20210517/x64/codeql/codeql database run-queries --ram=5929 --threads=2 /home/runner/work/_temp/codeql_databases/csharp --min-disk-free=1024 -v /home/runner/work/_temp/codeql_databases/csharp-queries-builtin.qls
  Could not determine the total number of lines of code in this repository. Because of this, it will not be possible to compare the number of lines of code analyzed by code scanning with the total number of lines of code in the repository. This will not affect the results produced by code scanning. If you have any questions, you can raise an issue at https://github.com/github/codeql-action/issues. Please include a link to the repository if public, or otherwise information about the code scanning workflow you are using.

And here's the link to one of the runs with the error: https://github.com/Skyluker4/UnityAssetReplacer/runs/2890183826

Many thanks for reaching out to us with this warning! As the log message says, the lines of code counting is for informational purposes only and will not affect the CodeQL analysis of your repository. It's quite a new feature, so we're still ironing out some small issues with it. Indeed, it seems not be correctly finding lines of code in C# files with the extension .cs because these are being incorrectly classified as Smalltalk for some reason!
I'll try to get to the bottom of what is going on for you and hopefully we can have our line counting working correctly on your repository (and other C# ones!) in not too long.

Thank you once again for reporting this! It should be fixed in our next release which should happen in the next a week or two :)

It works! I ran it from the main branch and there are no more errors.

That's great, thanks for trying it out from main!


Relabel presistent paths only at first boot

With this commit any RW paths gets a marker file added .relabelled to acknowledge this path got relabelled. At relabelling time if this marker file is found the path is omitted at relabelling stage. The idea is to only relabel persistent paths at first boot. This prevents relabelling persistent paths on each boot and gives the opportunity to select which paths to relabel by removing the markers.
Fixes https://github.com/rancher/elemental/issues/1417

This is slightly inspired by https://github.com/openSUSE/microos-tools/blob/master/selinux/98selinux-microos/selinux-microos-relabel.sh
I am even wondering if we should expand mount.selinux-relabel config option and instead of making it a boolean, define it as a string and except values like no|once|always or something similar. @frelon what you think?


If ProxySelector getDefault() throws AccessControlException, we should default to NullProxySelector

Hi there, I am trying to use okhttp but run into security permissions. It failed at ProxySelector getDefault with SecurityManager checkPermission on "getProxySelector".
For any case if getDefault throws exception, I think we should fall into NullProxySelector.
 this.proxySelector = ProxySelector.getDefault();
 if (this.proxySelector == null) {
               this.proxySelector = new NullProxySelector();
         }

Are you really sure you need a SecurityManager?

Are you really sure you need a SecurityManager?

Yes, even we can turn off security manager in DEV but in PROD we need it.

Tell me more. If your production environment relies on that security manager for security, I have bad news.

Tell me more. If your production environment relies on that security manager for security, I have bad news.

We have a java jar deployed in Apigee Edge(Cloud based). Apigee itself has some java security permissions setup. https://docs.apigee.com/api-platform/reference/java-permission-reference Unfortunately, we cannot override any security policy there.
I think we at least need a way to construct OkHttpClient.java with no proxy in okhttp lib.

Gotcha. Hopefully they aren't using that for real security, as it doesn't offer any.
Does providing a ProxySelector explicitly work around the problem?

Gotcha. Hopefully they aren't using that for real security, as it doesn't offer any.
Does providing a ProxySelector explicitly work around the problem?
Hmm, not quite sure. We don't have control over Apigee deployment JVM .

Gotcha. Hopefully they aren't using that for real security, as it doesn't offer any.
Does providing a ProxySelector explicitly work around the problem?

You should be able to configure a ProxySelector manually in your OkHttpClient.Builder().

Unfortunately, we are using OkHttp indirectly via another library that we do not control, so we cannot modify the construction of the OkHttp client. Given how widely OkHttp is wrapped by other libraries, I would think that this is a pretty common use case.
When the proxy selected permission is denied by a security manager, it is likely that the only reasonable option is a null proxy selector.  I would argue that a warning in the logs is more appropriate than failure.

It's the public API, so maybe worth supporting?  Jesse, would you take a PR?
https://docs.oracle.com/javase/7/docs/api/java/net/ProxySelector.html#getDefault()
Throws:
SecurityException - If a security manager has been installed and it denies NetPermission("getProxySelector")

That whole situation sounds just painful. We can probably alleviate some of it with a catch clause. But you should avoid the harmful SecurityManager and you should get your libraries to let you configure the OkHttpClient that they use.

That whole situation sounds just painful. We can probably alleviate some of it with a catch clause. But you should avoid the harmful SecurityManager and you should get your libraries to let you configure the OkHttpClient that they use.

Unfortunately, we do not control the provided JVM and cannot avoid the SecurityManager. Also we use OkHttpClient indirectly from another library as mentioned above.

https://github.com/square/okhttp/pull/5332


[SPARK-37777][SQL] Update the SQL syntax of SHOW FUNCTIONS

What changes were proposed in this pull request?

The SQL syntax of SHOW FUNCTIONS is very weird today. If you want to specify the database to list functions, you need to write SHOW FUNCTIONS LIKE db.f1. This is inconsistent with SHOW TABLES and SHOW VIEWS.
This PR proposes to follow SHOW TABLES and SHOW VIEWS, and support SHOW FUNCTIONS FROM/IN db LIKE pattern. To keep backward compatibility, the legacy syntax is still supported, but it can't be used together with the new FROM/IN db clause.
Why are the changes needed?

Be consistent with SHOW TABLES and SHOW VIEWS
Does this PR introduce any user-facing change?

Yea, as it extends the SHOW FUNCTIONS SQL syntax.
How was this patch tested?

updated tests

cc @MaxGekk @imback82 @viirya

Do we need to add new syntax to migration guide?

IIUC migration guide is for breaking changes, not new features. Since this PR doesn't break anything, we don't need to update migration guide.

thanks for the review, merging to master!


Error while faking System.Diagnostics.Process

I get the following error:
System.IO.FileLoadException: Loading this assembly would produce a different grant set from other instances. (Exception from HRESULT: 0x80131401)
   at System.Diagnostics.Process.TryPrigDispose(Process , Boolean )
   at System.Diagnostics.Process.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Finalize()
   at System.Diagnostics.Process.TryPrigDispose(Process , Boolean )
   at System.Diagnostics.Process.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Finalize()
========== Run test finished: 1 run (0:00:04.3713273) ==========

The error seems to occur at a line with:
var process = new System.Diagnostics.Process();

The code in the test leading up to it was:
            using (new IndirectionsContext())
            {
                PProcess.BeginErrorReadLine().Body = (Process p) => { };
                PProcess.BeginOutputReadLine().Body = (Process p) => { };
                PProcess.PriorityClassSetProcessPriorityClass().Body = (Process p, ProcessPriorityClass c) =>
                {
                    Assert.AreEqual(c, config.priorityClass);
                };
                PProcess.Start().Body = (System.Diagnostics.Process process) =>
                {

It seems as though Process is still trying to set up something internally. How do I shim over the entire process class?

I tried to reproduce the exception, but I couldn't. Hmm...
I guess the above setup code might not be the real cause because the stack trace indicates the location that is NOT in the code(System.Diagnostics.Process.TryPrigDispose(Process, Boolean) is generated automatically by Prig when you setup the Indirection Stub Setting of System.Diagnostics.Process.Dispose(Boolean) method).
Could you try the following steps to find what is the real cause?

Remove the Indirection Stub Setting of System.Diagnostics.Process.Dispose(Boolean) method from System.v4.0.30319.v<IP_ADDRESS>.prig.
Enclose Process object by using statement.
Call Process.WaitForExit method explicitly.

Hm fixed it after a bit of mucking around. Can't reproduce it consistently myself.


Improve sqlalchemy.ext.orderinglist types

Description
I've expanded the type hints for the ordering list while testing for https://github.com/sqlalchemy/sqlalchemy/issues/6205.
Checklist
This pull request is:

[ ] A documentation / typographical error fix

Good to go, no issue or tests are needed

[x] A short code fix

please include the issue number, and create an issue if none exists, which
must include a complete example of the issue.  one line code fixes without an
issue and demonstration will not be accepted.
Please include: Fixes: #<issue number> in the commit message
please include tests.   one line code fixes without tests will not be accepted.

[ ] A new feature implementation

please include the issue number, and create an issue if none exists, which must
include a complete example of how the feature would look.
Please include: Fixes: #<issue number> in the commit message
please include tests.

Have a nice day!

Please make sure all functions have a return value

thanks!


[GraphX][SPARK-2245] add a materialize method to materialize VertexRDD by calling RDD's count

Seems one can not materialize VertexRDD by simply calling count method, which is overridden by VertexRDD. But if you call RDD's count, it could materialize it.
Is this a feature that designed to get the count without materialize VertexRDD? If so, do you guys think it is necessary to add a materialize method to VertexRDD?
By the way, does count() is the cheapest way to materialize a RDD? Or it just cost the same resources like other actions?
Best,

@ankurdave @bxshi What's the status of this PR?
It hasn't been updated in a while, though it looks simple enough from a review standpoint.

It's not as simple as I thought...
Here is my reply on JIRA about this PR
I edited my original comment to add the updates, but I do not know if you can get them via email. So I resubmit it again. Hope that won't bother you. Ankur Dave
Hi Ankur Dave, I changed my pull request. But there is another exception, ShippableVertexPartition is not serializable. So I serialized it, but there is another exception org.apache.spark.graphx.impl.RoutingTablePartition is not serializable. Then I serialized it again, but on iteration 2 there will be an exception: org.apache.spark.graphx.impl.ShippableVertexPartition cannot be cast to scala.Tuple2
The code I'm using are:
val conf = new SparkConf().setAppName("HDTM")
.setMaster("local[4]")
val sc = new SparkContext(conf)
sc.setCheckpointDir("./checkpoint")
val v = sc.parallelize(Seq[(VertexId, Long)]((0L, 0L), (1L, 1L), (2L, 2L)))
val e = sc.parallelize(Seq[Edge[Long]](Edge(0L, 1L, 0L), Edge(1L, 2L, 1L), Edge(2L, 0L, 2L)))
var g = Graph(v, e)
val vertexIds = Seq(0L, 1L, 2L)
var prevG: Graph[VertexId, Long] = null
for (i <- 1 to 2000) {
vertexIds.toStream.foreach(id =>
{ prevG = g g = Graph(g.vertices, g.edges) g.vertices.cache() g.edges.cache() prevG.unpersistVertices(blocking = false) prevG.edges.unpersist(blocking = false) }
)
g.vertices.checkpoint()
g.edges.checkpoint()
g.edges.count()
g.vertices.count()
println(s"$
{g.vertices.isCheckpointed}
$
{g.edges.isCheckpointed}
")
println(" iter " + i + " finished")
}
println(g.vertices.collect().mkString(" "))
println(g.edges.collect().mkString(" "))

Am I on the right track? Or Should there be another way to change it?

Can one of the admins verify this patch?

Can one of the admins verify this patch?