Orphaned Service Catalog objects after Namespace deletion during Service Catalog API server outage

Bug Report
What happened:
During Service Catalog API server outage, namespace deletion proceeds without waiting for Service Catalog objects to finish their deletion.
What you expected to happen:

Namespace shouldn't proceed with deletion during Service Catalog outage. We may add Service Catalog finalizer to every namespace to achieve this.
Otherwise, when we reconcile Service Catalog object and detect that its parent namespace is missing, we should probably initiate its deletion?

How to reproduce it (as minimally and precisely as possible):

Create namespace ns1
Create a ServiceInstance object in namespace ns1
Undeploy Service Catalog API server (you could simply set replicas: 0 in the deployment)
Delete ns1 namespace
Deploy Service Catalog API server
Namespace ns1 is missing, but ServiceInstance object in this namespace still exists. In some cases, it is impossible to delete this object until you manually create ns1 namespace again.

The issue is caused by the Namespace controller deletion logic:

Namespace controller fetches a list of resource types (GroupVersionResource) supported by every API server via fetching an APIResourceList object, see https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/namespace/deletion/namespaced_resources_deleter.go#L484
Then, for every GroupVersionResource it will issue a delete request to a corresponding API server
Once all known resources deleted, Namespace controller will remove a finalizer from the Namespace and will proceed with deletion.

In the happy path it works fine, but when API server goes down, it unregisters itself from the list of "API versions", which you can manually check via kubectl api-versions (for Service Catalog you'll find servicecatalog.k8s.io/v1beta1 in this list). Since a particular apiVersion is not registered anymore, Namespace controller doesn't "see" corresponding types of resources anymore, and leaves them untouched.
@liggitt I believe this logic (and corresponding issue) applies not only to Service Catalog API server, but to API Extensions (CRD) API server as well, and to any other non-core API server. What is the recommended approach to correctly handle this situation?

While this issue may sound like a very rare corner-case, in reality it's easy to run into this issue with long asynchronous operations and finalizers. Example:

ServiceInstance is created, and asynchronous provisioning is started.
Namespace is marked for deletion, and remains in Terminating state, because ServiceInstance has a kubernetes/service-catalog finalizer which blocks its deletion.
Once there is an outage of Service Catalog API server (e.g. possibly during update with a downtime), namespace gets deleted.
Service Catalog continues processing ServiceInstance up until it is ready to remove the finalizer, but the namespace is gone already.

Why does step 3 happen if svc-cat didn’t complete its job yet?
Sent from my iPad

On Aug 2, 2018, at 6:33 AM, Nail Islamov<EMAIL_ADDRESS>wrote:
While this issue may sound like a very rare corner-case, in reality it's easy to run into this issue with long asynchronous operations and finalizers. Example:
ServiceInstance is created, and asynchronous provisioning is started.
Namespace is marked for deletion, and remains in Terminating state, because ServiceInstance has a kubernetes/service-catalog finalizer which blocks its deletion.
Once there is an outage of Service Catalog API server (e.g. possibly during update with a downtime), namespace gets deleted.
Service Catalog continues processing ServiceInstance up until it is ready to remove the finalizer, but the namespace is gone already.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Ah never mind, was reading emails out of order
Sent from my iPad

On Aug 2, 2018, at 6:25 AM, Nail Islamov<EMAIL_ADDRESS>wrote:
The issue is caused by the Namespace controller deletion logic:
Namespace controller fetches a list of resource types (GroupVersionResource) supported by every API server via fetching an APIResourceList object, see https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/namespace/deletion/namespaced_resources_deleter.go#L484
Then, for every GroupVersionResource it will issue a delete request to a corresponding API server
Once all known resources deleted, Namespace controller will remove a finalizer from the Namespace and will proceed with deletion.
In the happy path it works fine, but when API server goes down, it unregisters itself from the list of "API versions", which you can manually check via kubectl api-versions (for Service Catalog you'll find servicecatalog.k8s.io/v1beta1 in this list). Since a particular apiVersion is not registered anymore, Namespace controller doesn't "see" corresponding types of resources anymore, and leaves them untouched.
@liggitt I believe this logic (and corresponding issue) applies not only to Service Catalog API server, but to API Extensions (CRD) API server as well, and to any other non-core API server. What is the recommended approach to correctly handle this situation?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

when API server goes down, it unregisters itself from the list of "API versions"

What version are you running? This should be leaving the API version registered and return 503 errors when accessed, which would fail resource discovery and block namespace deletion

What version are you running?

We are running k8s 1.11
@liggitt Based on the behavior I see, I think the 503 change you referenced only applies to a proxy that intercepts request to an actual API server, i.e. when you will try to fetch an APIResourceList from a missing API server, you will get 503 from aggregator without it sending actual request to the underlying server.
Namespace controller is not sending a request directly to the Service Catalog API server though. It just sends a single request to the aggregated server discovery endpoint, which handles unavailable API servers differently: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_apis.go#L95-L97
i.e. it just skips APIService objects that don't have Available: true condition.

When Service Catalog API server is available, corresponding APIService status is:
status:
  conditions:
  - lastTransitionTime: 2018-08-02T14:20:42Z
    message: all checks passed
    reason: Passed
    status: "True"
    type: Available

And kubectl api-versions result contains Service Catalog:
❯ kubectl api-versions | grep servicecatalog
servicecatalog.k8s.io/v1beta1

When I scale Service Catalog deployment to zero, I get the status:
status:
  conditions:
  - lastTransitionTime: 2018-08-02T14:19:32Z
    message: endpoints for service/service-catalog-apiserver in "service-catalog"
      have no addresses
    reason: MissingEndpoints
    status: "False"
    type: Available

But kubectl api-versions returns the result without errors, it's just missing the Service Catalog group.
As a result, Namespace controller doesn't know about the existence of Service Catalog API server, so it proceeds with the namespace deletion without blocking.

It is possible that the command line api-versions command is tolerating a partial discovery error. The namespace controller does not. What is the content of the following endpoints:
/apis
/apis/servicecatalog.k8s.io
/apis/servicecatalog.k8s.io/v1beta1

Raw output of kubectl apiversions

❯ k api-versions -v=8
I0802 20:00:35.290924   34275 round_trippers.go:383] GET https://kubernetes.ap-southeast-2.paas-playground.kitt-inf.net/api
I0802 20:00:35.290945   34275 round_trippers.go:390] Request Headers:
I0802 20:00:35.290952   34275 round_trippers.go:393]     Accept: application/json, */*
I0802 20:00:35.290958   34275 round_trippers.go:393]     User-Agent: kubectl/v1.10.2 (darwin/amd64) kubernetes/81753b1
I0802 20:00:37.550331   34275 round_trippers.go:408] Response Status: 200 OK in 2259 milliseconds
I0802 20:00:37.550354   34275 round_trippers.go:411] Response Headers:
I0802 20:00:37.550363   34275 round_trippers.go:414]     Content-Length: 135
I0802 20:00:37.550373   34275 round_trippers.go:414]     Date: Thu, 02 Aug 2018 15:00:37 GMT
I0802 20:00:37.550382   34275 round_trippers.go:414]     Audit-Id: 48a853ba-94a6-407d-8c4d-229b54718741
I0802 20:00:37.550389   34275 round_trippers.go:414]     Content-Type: application/json
I0802 20:00:37.556333   34275 request.go:874] Response Body: {"kind":"APIVersions","versions":["v1"],"serverAddressByClientCIDRs":[{"clientCIDR":"<IP_ADDRESS>/0","serverAddress":"<IP_ADDRESS>:443"}]}
I0802 20:00:37.557482   34275 round_trippers.go:383] GET https://kubernetes.ap-southeast-2.paas-playground.kitt-inf.net/apis
I0802 20:00:37.557497   34275 round_trippers.go:390] Request Headers:
I0802 20:00:37.557504   34275 round_trippers.go:393]     Accept: application/json, */*
I0802 20:00:37.557511   34275 round_trippers.go:393]     User-Agent: kubectl/v1.10.2 (darwin/amd64) kubernetes/81753b1
I0802 20:00:37.970245   34275 round_trippers.go:408] Response Status: 200 OK in 412 milliseconds
I0802 20:00:37.970272   34275 round_trippers.go:411] Response Headers:
I0802 20:00:37.970280   34275 round_trippers.go:414]     Audit-Id: a1c59dfe-0a5d-4732-bd1d-9a3708ce081b
I0802 20:00:37.970288   34275 round_trippers.go:414]     Content-Type: application/json
I0802 20:00:37.970295   34275 round_trippers.go:414]     Date: Thu, 02 Aug 2018 15:00:37 GMT
I0802 20:00:37.989835   34275 request.go:874] Response Body: {"kind":"APIGroupList","apiVersion":"v1","groups":[{"name":"apiregistration.k8s.io","versions":[{"groupVersion":"apiregistration.k8s.io/v1","version":"v1"},{"groupVersion":"apiregistration.k8s.io/v1beta1","version":"v1beta1"}],"preferredVersion":{"groupVersion":"apiregistration.k8s.io/v1","version":"v1"}},{"name":"extensions","versions":[{"groupVersion":"extensions/v1beta1","version":"v1beta1"}],"preferredVersion":{"groupVersion":"extensions/v1beta1","version":"v1beta1"}},{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"},{"groupVersion":"apps/v1beta2","version":"v1beta2"},{"groupVersion":"apps/v1beta1","version":"v1beta1"}],"preferredVersion":{"groupVersion":"apps/v1","version":"v1"}},{"name":"events.k8s.io","versions":[{"groupVersion":"events.k8s.io/v1beta1","version":"v1beta1"}],"preferredVersion":{"groupVersion":"events.k8s.io/v1beta1","version":"v1beta1"}},{"name":"authentication.k8s.io","versions":[{"groupVersion":"authentication.k8s.io/v1","version":"v1"},{"groupVersion":"authenticati [truncated 5143 chars]
admissionregistration.k8s.io/v1beta1
apiextensions.k8s.io/v1beta1
apiregistration.k8s.io/v1
apiregistration.k8s.io/v1beta1
apps/v1
apps/v1beta1
apps/v1beta2
authentication.k8s.io/v1
authentication.k8s.io/v1beta1
authorization.k8s.io/v1
authorization.k8s.io/v1beta1
autoscaling/v1
autoscaling/v2beta1
batch/v1
batch/v1beta1
batch/v2alpha1
bitnami.com/v1alpha1
certificates.k8s.io/v1beta1
clusterregistry.k8s.io/v1alpha1
composition.voyager.atl-paas.net/v1
contour.heptio.com/v1beta1
creator.voyager.atl-paas.net/v1
events.k8s.io/v1beta1
extensions/v1beta1
formation.voyager.atl-paas.net/v1
metrics.k8s.io/v1beta1
networking.k8s.io/v1
ops-gateway.voyager.atl-paas.net/v1
ops.voyager.atl-paas.net/v1
orchestration.voyager.atl-paas.net/v1
policy/v1beta1
rbac.authorization.k8s.io/v1
rbac.authorization.k8s.io/v1alpha1
rbac.authorization.k8s.io/v1beta1
reporter.voyager.atl-paas.net/v1
scheduling.k8s.io/v1beta1
servicecatalog.k8s.io/v1beta1
smith.atlassian.com/v1
storage.k8s.io/v1
storage.k8s.io/v1beta1
v1

Namespace controller:

Fetches initial list of GroupVersionResource from https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/namespace/deletion/namespaced_resources_deleter.go#L484
Which is implemented here: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/core.go#L265
That invokes this method: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/discovery/discovery_client.go#L257
Which invokes /api endpoint https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/discovery/discovery_client.go#L138

I suspect that this call returns already filtered list of API groups (doesn't contain Service Catalog API group), but I could be wrong. I will check this later.

Can you fetch the non-truncated output?

@liggitt sure:
/apis
Response (servicecatalog is missing):
{
  "kind": "APIGroupList",
  "apiVersion": "v1",
  "groups": [
    {
      "name": "apiregistration.k8s.io",
      "versions": [
        {
          "groupVersion": "apiregistration.k8s.io/v1",
          "version": "v1"
        },
        {
          "groupVersion": "apiregistration.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "apiregistration.k8s.io/v1",
        "version": "v1"
      }
    },
    {
      "name": "extensions",
      "versions": [
        {
          "groupVersion": "extensions/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "extensions/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "apps",
      "versions": [
        {
          "groupVersion": "apps/v1",
          "version": "v1"
        },
        {
          "groupVersion": "apps/v1beta2",
          "version": "v1beta2"
        },
        {
          "groupVersion": "apps/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "apps/v1",
        "version": "v1"
      }
    },
    {
      "name": "events.k8s.io",
      "versions": [
        {
          "groupVersion": "events.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "events.k8s.io/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "authentication.k8s.io",
      "versions": [
        {
          "groupVersion": "authentication.k8s.io/v1",
          "version": "v1"
        },
        {
          "groupVersion": "authentication.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "authentication.k8s.io/v1",
        "version": "v1"
      }
    },
    {
      "name": "authorization.k8s.io",
      "versions": [
        {
          "groupVersion": "authorization.k8s.io/v1",
          "version": "v1"
        },
        {
          "groupVersion": "authorization.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "authorization.k8s.io/v1",
        "version": "v1"
      }
    },
    {
      "name": "autoscaling",
      "versions": [
        {
          "groupVersion": "autoscaling/v1",
          "version": "v1"
        },
        {
          "groupVersion": "autoscaling/v2beta1",
          "version": "v2beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "autoscaling/v1",
        "version": "v1"
      }
    },
    {
      "name": "batch",
      "versions": [
        {
          "groupVersion": "batch/v1",
          "version": "v1"
        },
        {
          "groupVersion": "batch/v1beta1",
          "version": "v1beta1"
        },
        {
          "groupVersion": "batch/v2alpha1",
          "version": "v2alpha1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "batch/v1",
        "version": "v1"
      }
    },
    {
      "name": "certificates.k8s.io",
      "versions": [
        {
          "groupVersion": "certificates.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "certificates.k8s.io/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "networking.k8s.io",
      "versions": [
        {
          "groupVersion": "networking.k8s.io/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "networking.k8s.io/v1",
        "version": "v1"
      }
    },
    {
      "name": "policy",
      "versions": [
        {
          "groupVersion": "policy/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "policy/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "rbac.authorization.k8s.io",
      "versions": [
        {
          "groupVersion": "rbac.authorization.k8s.io/v1",
          "version": "v1"
        },
        {
          "groupVersion": "rbac.authorization.k8s.io/v1beta1",
          "version": "v1beta1"
        },
        {
          "groupVersion": "rbac.authorization.k8s.io/v1alpha1",
          "version": "v1alpha1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "rbac.authorization.k8s.io/v1",
        "version": "v1"
      }
    },
    {
      "name": "storage.k8s.io",
      "versions": [
        {
          "groupVersion": "storage.k8s.io/v1",
          "version": "v1"
        },
        {
          "groupVersion": "storage.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "storage.k8s.io/v1",
        "version": "v1"
      }
    },
    {
      "name": "admissionregistration.k8s.io",
      "versions": [
        {
          "groupVersion": "admissionregistration.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "admissionregistration.k8s.io/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "apiextensions.k8s.io",
      "versions": [
        {
          "groupVersion": "apiextensions.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "apiextensions.k8s.io/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "scheduling.k8s.io",
      "versions": [
        {
          "groupVersion": "scheduling.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "scheduling.k8s.io/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "creator.voyager.atl-paas.net",
      "versions": [
        {
          "groupVersion": "creator.voyager.atl-paas.net/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "creator.voyager.atl-paas.net/v1",
        "version": "v1"
      }
    },
    {
      "name": "reporter.voyager.atl-paas.net",
      "versions": [
        {
          "groupVersion": "reporter.voyager.atl-paas.net/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "reporter.voyager.atl-paas.net/v1",
        "version": "v1"
      }
    },
    {
      "name": "ops-gateway.voyager.atl-paas.net",
      "versions": [
        {
          "groupVersion": "ops-gateway.voyager.atl-paas.net/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "ops-gateway.voyager.atl-paas.net/v1",
        "version": "v1"
      }
    },
    {
      "name": "composition.voyager.atl-paas.net",
      "versions": [
        {
          "groupVersion": "composition.voyager.atl-paas.net/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "composition.voyager.atl-paas.net/v1",
        "version": "v1"
      }
    },
    {
      "name": "formation.voyager.atl-paas.net",
      "versions": [
        {
          "groupVersion": "formation.voyager.atl-paas.net/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "formation.voyager.atl-paas.net/v1",
        "version": "v1"
      }
    },
    {
      "name": "ops.voyager.atl-paas.net",
      "versions": [
        {
          "groupVersion": "ops.voyager.atl-paas.net/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "ops.voyager.atl-paas.net/v1",
        "version": "v1"
      }
    },
    {
      "name": "orchestration.voyager.atl-paas.net",
      "versions": [
        {
          "groupVersion": "orchestration.voyager.atl-paas.net/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "orchestration.voyager.atl-paas.net/v1",
        "version": "v1"
      }
    },
    {
      "name": "smith.atlassian.com",
      "versions": [
        {
          "groupVersion": "smith.atlassian.com/v1",
          "version": "v1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "smith.atlassian.com/v1",
        "version": "v1"
      }
    },
    {
      "name": "bitnami.com",
      "versions": [
        {
          "groupVersion": "bitnami.com/v1alpha1",
          "version": "v1alpha1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "bitnami.com/v1alpha1",
        "version": "v1alpha1"
      }
    },
    {
      "name": "clusterregistry.k8s.io",
      "versions": [
        {
          "groupVersion": "clusterregistry.k8s.io/v1alpha1",
          "version": "v1alpha1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "clusterregistry.k8s.io/v1alpha1",
        "version": "v1alpha1"
      }
    },
    {
      "name": "contour.heptio.com",
      "versions": [
        {
          "groupVersion": "contour.heptio.com/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "contour.heptio.com/v1beta1",
        "version": "v1beta1"
      }
    },
    {
      "name": "metrics.k8s.io",
      "versions": [
        {
          "groupVersion": "metrics.k8s.io/v1beta1",
          "version": "v1beta1"
        }
      ],
      "preferredVersion": {
        "groupVersion": "metrics.k8s.io/v1beta1",
        "version": "v1beta1"
      }
    }
  ]
}

/apis/servicecatalog.k8s.io
Returns 404
/apis/servicecatalog.k8s.io/v1beta1
Returns 503

k8s exact version:
Server Version: version.Info{Major:"1", Minor:"11", GitVersion:"v1.11.1", GitCommit:"b1b29978270dc22fecc592ac55d903350454310a", GitTreeState:"clean", BuildDate:"2018-07-17T18:43:26Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"linux/amd64"}

From kubectl api-versions -v=8 I see that it's using /api and /apis endpoints. The second is missing servicecatalog.k8s.io API group in the response .
Namespace controller does the same (plus extra methods based on the list returned in /apis):

Function for fetching a list is set there: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/core.go#L265

Which fetches a list of API groups in the first line: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/discovery/discovery_client.go#L257

Which internally invokes /api and then /apis: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/discovery/discovery_client.go#L149

The issue is fixed in Kubernetes master, will keep this issue open until it gets cherry picked to k8s 1.11, released and we update dependency to a newer version.
See https://github.com/kubernetes/kubernetes/pull/67154

@nilebox Sorry -- updating my branch closed this by accident.

k8s 1.11.3 has just been released https://github.com/kubernetes/kubernetes/releases/tag/v1.11.3

We can close this issue without updating dependencies to kubernetes 1.11.3, since the bug is in the Kubernetes itself, not in the Service Catalog using the library.


—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/USGS-EROS/lcmap-pyccd/pull/2#event-887279667, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AOWsAAq_1xcNojLH_ifvBFiF-8a4DEzDks5rGExQgaJpZM4LIH2d
.

--
Brian N. Davis
Principal Software Engineer
SGT, Inc.
Contractor to the U.S. Geological Survey (USGS)
Earth Resources Observation and Science (EROS) Center
47914 252ndStreet
Sioux Falls, SD 57198
Phone:<PHONE_NUMBER>
Fax:<PHONE_NUMBER>
43.735926, -96.625324
NOTE – I have a new E-mail address as of April 4, 2016:
<EMAIL_ADDRESS><EMAIL_ADDRESS>Please update your contact list accordingly!
NOTICE:  This email may contain confidential, proprietary, or competition
sensitive bid or proposal procurement information.  Unauthorized disclosure
of this information may carry criminal penalties as set forth in the
Procurement Integrity Act, 41 U.S.C. 423, as amended.  Further, the
unauthorized disclosure of certain commercial information by civil servants
may result in fines or imprisonment under the Trade Secrets Act (18 U.S.C.
1905).  If you have received this information in error, please delete it,
including all copies, and notify the sender of the error immediately.


Punctuation and grammar fixes

Multiple grammar, repeated words, and punctuation fixed.

 Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.:x: Jcoetsee sign nowYou have signed the CLA already but the status is still pending? Let us recheck it.

@dianef77 - can you please assist to check, approve and merge the content for this PR based on issue #94?
Thanks.

@Jcoetsee - please recheck on the changes you made on the PR.


NugetCommand@2 versioningScheme: byBuildNumber truncates a build number with a hyphen

Required Information
Entering this information will route you directly to the right team and expedite traction.
Type: Bug
Enter Task Name: NugetCommand@2
Environment

Server - Azure Pipelines
https://dev.azure.com/DenverHealth-EpicCogito/Epic/_build/results?buildId=34892&view=results

Agent - Hosted or Private:
Hosted: windows-latest

Issue Description
I tried to use the built in versioning scheme functionality in NugetCommand@2's pack functionality with the following yaml
- task: NuGetCommand@2
  displayName: Nuget Pack
  inputs:
    command: 'pack'
    packagesToPack: '$(Pipeline.Workspace)/b/dbatools.nuspec'
    versioningScheme: 'byBuildNumber'

I configured my build number using this yaml:
name: 1.0.116-D$(BuildID)
Unfortunately, the version on the package is 1.0.116 :(

When I used the byPreReleaseNumber functionality it worked:

I am unable to use the build date functionality because PowerShell only allows you to have one hyphen in a pre-release package.
Task logs
logs_34892.zip

Hey,
We recommend you to use nuget authenticate
see this link for more info
https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/package/nuget-authenticate?view=azure-devops.
Thanks

I think we're still affected by, but working around this.  Not sure how the authentication would cause a variable value to not be what we expect.

I think we're still affected by, but working around this.

Or even better ... add a new case named something like bySemVerBuildNumber, and check if it is semver 2.0 compatible, else fail the task and thereby the build.


Remove redundant LICENSE file, and fix the stray git text in next-steps.md

I'm not sure if some of these are intentional and work with the template or azd. If so, will totally close this PR.


Remove Central and simpify/unify API

Decouples components from Central paradigm (Central has been removed).
API was simplified and unified by making the following changes:

MacOS target holds an internal CentralManager
Android target uses AndroidX startup to register an Initializer (and obtain application Context)

Scanner is no longer scoped, so Peripheral remains the only Coroutine scoped component. This allows library consumers more flexibility about the lifecycle of the various components.
Updated README can be viewed here.

Note to self: let #23 merge first, then be sure to properly setup scanner function for JS target.

General question for reviewers:
A Scanner can be obtained via scanner() function, but it is common to have a function "act as a constructor" (e.g. Json in kotlinx.serialization and similar examples can be found in Coroutines library). As such, I'd also prefer Scanner() function so that calling it "feels" like constructor invocation, but it clashes with interface with same name.
Q: Is Scanner function (that feels like a constructor) a good idea?
Q: If so, what should Scanner interface be renamed to?

Q: Is Scanner function (that feels like a constructor) a good idea?
Q: If so, what should the current Scanner interface be renamed to?

Answering this before I do my review.
I'm totally fine with a fun Scanner(...): Scanner to act as the constructor. Complete with the exact name match on the interface. This is exactly the pattern you see on a ton of coroutines stuff, such as MutableStateFlow.

Complete with the exact name match on the interface.

🤦  I'm not sure what I was doing wrong, but when I tried this earlier it wasn't compiling. Seems to be working now, this was my preferred approach. So, I'm glad it seems to be working now. 😄


[BUG] - Flaky jupyterhub_ssh test

OS system and architecture in which you are running QHub
Ubuntu
Expected behavior
The tests_deployment/test_jupyterhub_ssh.py::test_print_jupyterhub_ssh test that is run during the Kubernetes test CI seems to occasionally fail for unknown reasons. Rerunning the CI usually results in the test passing.
See recent logs for more info.
Actual behavior
I expect the behavior to be consistent, either pass consistently or fail consistently.
I have run into this issue three times in the past week and each time the test appeared to be flaky as running the same test again results in it passing.
How to Reproduce the problem?
Not known at the moment. Reproducibility seems to be the issue.
Command output
No response
Versions and dependencies used.
No response
Compute environment
minikube
Integrations
No response
Anything else?
No response

I would suggest increasing the @pytest.mark.timeout(120) to 300s


Remove no-longer-needed extraneous go subdirs

Issue #3667
Brought up to date with commit 3cbaf56.
Mostly scripted out, so feel free to commit if/when convenient.

I would approve the removal of the js impl at this point. I'm opposed to changing the go structure at the moment.

Splitting it up.

jenkins: test this


Why are JS files ignored in the Bower package?

Hello,
Since https://github.com/twitter/twemoji/commit/590d7a326861867964a4c1a6f1d44505fffa302b, twemoji.js is excluded from the Bower package. As I use Bower in order to easily get JS libraries, this breaks my workflow.
Is there any particular reason these files are ignored?

files seem to be there, which change broke your build? I might double check later on (this evening)
Apologies for any inconvenience

It seems the files were meant to be ignored since 9edd9dd4a8c54533678cb04b658c305c2d1ccfaf but the syntax was wrong so 590d7a326861867964a4c1a6f1d44505fffa302b fixed it.
Having these files in the ignore property means Bower removes them after install (but it still downloads them).
The same thing applies to the images (16x16, etc) folders.

would ignoring everything but version 2 be OK? the main file is version 2 and bringing in all V1 assets is pointless, right?

Yes, it would be OK for me.

I will try to make it work this evening (I don't use/know bower that much so a PR would also be welcome)


Add Rubik's AI?

https://rubiks.ai

Thanks!
Please tell me if you know of anything written on it, from the builders or externally.
Here is what I'm adding for now:
{
    "name": "Rubik's AI",
    "id": "rubiks-ai",
    "characteristics": {
      "output": "generated response"
    },
    "search_link": "https://rubiks.ai/search/?q=%s",
    "base_url": "https://rubiks.ai",
    "default_placeholder": "How can I help you?",
    "twitter_link": "https://twitter.com/RubiksAI"
  }
]

You can see the fields I use in the src/data/systems.json file.

That looks accurate. Your collection is quite impressive!


testsuitesmanagement not executable anymore after sphinx maintenance

Hi Son,
Holger maintained the sphinx project for the testsuitesmanagement .
Unfortunately it was required to change include paths (absolute to releative, ...).
As a result currently testsuitesmanagement is not running anymore.
Please maintain the code and please take very much care that the sphinx based html and latex build works still properly after your changes.
Please also refer to the commit messages from Holger:
"* !!! CAUTION: Import paths modified to resolve Sphinx errors (positions marked with "# Sphinx fix to be verified"). This modification needs to be verified !!!

!!! CAUTION: Some docstrings commented out to resolve Sphinx errors (rst syntax issues). These docstrings need to be reworked !!! "

Thank you,
Thomas

Hi Son,
!!! please give me some more time before you start any activity !!! I overlooked some interdependencies. I am still learning but most probably the changes I made in your code are not necessary. I will come back to you soon when I have a valid solution.

Hi Son,
O.K. nothing to do here for the moment for you. Please wait for Holgers trigger.
Thank you,
Thomas

I installed https://github.com/test-fullautomation/robotframework-testsuitesmanagement/pull/9
based on alpha__RobotFramework_setup_vscode_apertis_rfw4.1__<IP_ADDRESS>.exe
Executing a robot file causes the following error message:
20220308 10:16:43.762 - FAIL - NameError: name 'RobotFramework_Testsuites' is not defined
20220308 10:16:43.763 - DEBUG - Traceback (most recent call last):
File "D:\RobotFramework\python39\lib\site-packages\RobotFramework_Testsuites_init_.py", line 63, in run_keyword
return DynamicCore.run_keyword(self, name, args, kwargs)
File "D:\RobotFramework\python39\lib\site-packages\robotlibcore.py", line 94, in run_keyword
return self.keywords[name](*args, **(kwargs or {}))
File "D:\RobotFramework\python39\lib\site-packages\RobotFramework_Testsuites\Keywords\CSetup.py", line 37, in testsuite_setup
if not RobotFramework_Testsuites.CTestsuitesCfg.oConfig.bLoadedCfg:

Hello Holger,
I have just test the functionality of Testsuites-Management package after your #9 was merged.
Unfortunately, robot run failed due to "NameError: name 'RobotFramework_Testsuites' is not defined"
In python module: ".../RobotFramework_Testsuites/Keywords/CSetup.py" line 17, could you please un-comment out "import RobotFramework_Testsuites" which you commented out in #6 ?

Thank you,
Son

I installed #9 based on alpha__RobotFramework_setup_vscode_apertis_rfw4.1__<IP_ADDRESS>.exe
Executing a robot file causes the following error message: 20220308 10:16:43.762 - FAIL - NameError: name 'RobotFramework_Testsuites' is not defined 20220308 10:16:43.763 - DEBUG - Traceback (most recent call last): File "D:\RobotFramework\python39\lib\site-packages\RobotFramework_Testsuites__init__.py", line 63, in run_keyword return DynamicCore.run_keyword(self, name, args, kwargs) File "D:\RobotFramework\python39\lib\site-packages\robotlibcore.py", line 94, in run_keyword return self.keywords[name](*args, **(kwargs or {})) File "D:\RobotFramework\python39\lib\site-packages\RobotFramework_Testsuites\Keywords\CSetup.py", line 37, in testsuite_setup if not RobotFramework_Testsuites.CTestsuitesCfg.oConfig.bLoadedCfg:

Hmm - maybe I overlooked this position while undoing my changes. When I uncomment this line I am able to execute my test and the test is passed. But nevertheless in between the following error message occurs:
[ ERROR ] Calling method '_start_suite' of listener 'LibListener' failed: AttributeError: type object 'SuiteStructureBuilder' has no attribute 'data_sources'
I execute robotframework-extensions-collection\test\01.test.pretty_print.robot in a simple command line (batch file).

Hi Son,
O.K. Thank you for your reply. n
Now I need your help!
Please try to get testsuites-management run again. Please do the required changes.
Holger doesn't know about the function of the module, only about the documentation generator part.
So therefore we need to iteratively correct till both, code and documentation generator is working properly.
So, please fix and create a new pull request. This will then go to Holger again for check.
Thank you,
Thomas

Hello Thomas,
I have just created the pull-request #10  to fix the issue "NameError: name 'RobotFramework_Testsuites' is not defined"
I also did the functionality test and selftest for Testsuites-Management package (everything are working fine).

Thank you,
Son

Hi Son,
Thank you, very good!
Thomas

Hi Holger,
I merged #10. Can you please check if Sphinx runs now properly.
(O.K. now we have also the new problem with the package naming)
Thank you,
Thomas

This issue is solved.


Fix span-labeling-datasets for spacy v3.5

Description

Fix span-labeling-datasets for spacy v3.5.
Types of change

Bug fixes.
Checklist

[x] I confirm that I have the right to submit this contribution under the project's MIT license.
[x] I ran the tests, and all new and existing tests passed.
[x] I ran the update scripts in the .github folder, and all the configs and docs are up-to-date.

It's very likely that someone is going to accidentally check in the filled config and then it won't be backwards compatible.

It's not great that you can't run the steps again in the same directory because of permissions errors related to untar-ing in temp/, and cleaning up the temporary state is difficult because some things are written to assets/`.


feat: onboarding mode, updates, and audit log events

Please describe the changes this PR makes and why it should be merged:
Split this up from #713 in the hopes of landing onboarding at some point this year
If applicable, please reference Discord API Docs PRs or commits that influenced this PR:

discord/discord-api-docs#6041
discord/discord-api-docs#6101

https://github.com/discordjs/discord.js/pull/9647#issuecomment-1641408397


How to use remarkable-katex plugin with docusaurus?

Is this a bug report?
Not sure. Perhaps I don't understand how the plugin service for docusaurus works.

Have you read the Contributing Guidelines on issues?
Yes
Environment
(OS, Node, npm, yarn)
MacOS, Node 8.9.1, NPM 5.6.0
Steps to Reproduce

I am trying to get the remarkable-katex plugin to work in my docusaurus site. I added it to the package. json:
{
  "scripts": {
    "examples": "docusaurus-examples",
    "start": "docusaurus-start",
    "build": "docusaurus-build",
    "publish-gh-pages": "docusaurus-publish",
    "write-translations": "docusaurus-write-translations",
    "version": "docusaurus-version",
    "rename-version": "docusaurus-rename-version"
  },
  "devDependencies": {
    "docusaurus": "^1.0.11",
    "remarkable-katex": "^1.0.1"
  }
}

Then I did an npm install:
~/g/o/d/website (master|●33✚11) $ npm install
npm WARN website No description
npm WARN website No repository field.
npm WARN website No license field.

up to date in 1.327s
~/g/o/d/website (master|●33✚11) $ npm list remarkable-katex
/Users/philipjohnson/github/openpowerquality/docusaurus/website
└──<EMAIL_ADDRESS>
~/g/o/d/website (master|●33✚11) $

Then I tried to run docusaurus:
npm start

Expected Behavior

I expected the system to work.
Actual Behavior

Instead, the npm start command failed with this error:
npm start

> @ start /Users/philipjohnson/github/openpowerquality/docusaurus/website
> docusaurus-start

Starting Docusaurus server on port 3000...
/Users/philipjohnson/github/openpowerquality/docusaurus/website/node_modules/docusaurus/lib/start-server.js:70
        throw ex;
        ^

TypeError: plugin is not a function
    at Remarkable.use (/Users/philipjohnson/github/openpowerquality/docusaurus/website/node_modules/remarkable/lib/index.js:122:3)
    at /Users/philipjohnson/github/openpowerquality/docusaurus/website/node_modules/docusaurus/lib/core/renderMarkdown.js:65:12
    at Array.forEach (<anonymous>)
    at new MarkdownRenderer (/Users/philipjohnson/github/openpowerquality/docusaurus/website/node_modules/docusaurus/lib/core/renderMarkdown.js:64:34)
    at Object.<anonymous> (/Users/philipjohnson/github/openpowerquality/docusaurus/website/node_modules/docusaurus/lib/core/renderMarkdown.js:81:24)
    at Module._compile (module.js:635:30)
    at loader (/Users/philipjohnson/github/openpowerquality/docusaurus/website/node_modules/babel-register/lib/node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/philipjohnson/github/openpowerquality/docusaurus/website/node_modules/babel-register/lib/node.js:154:7)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)

Reproducible Demo
(Paste the link to an example repo, including a siteConfig.js, and exact instructions to reproduce the issue.)
https://github.com/openpowerquality/docusaurus/tree/remarkable-katex

Hi @philipmjohnson the markdownPlugins field is supposed to take in an array of functions that receive the remarkable instance as a parameter. Refer to the example config
I fixed it for you in https://github.com/openpowerquality/docusaurus/pull/1. Have a go at it 😄

Wow!  Now that's service!  Thank you so much for the quick response. Things almost work perfectly now---KaTeX needs the DOCTYPE added to HTML files, but I see that you've just merged #383 yesterday so I will patiently await the next release for perfection.

@philipmjohnson I should be pushing another release sometime this week, possibly today or tomorrow.


Add CODE_OF_CONDUCT.md to export-ignore

Coverage remained the same at 100.0% when pulling 5acc289d711be80245182e2a23f68c6bb98258dc on adriansuter:patch-1 into 2e032a9f94d100c66c074026dd9404399a60f10f on slimphp:master.


Logging integration is only showing 'severe' category logs in Sentry

Just tried the new sentry_logger package. All installed fine etc.
When we tried some test log messages, the only one that made it through to Sentry was the "severe" category message. Is this by design ? Or do we need to set something up in Sentry to allow the different log categories to show up ?
var test = Logger("TestLoggerForSentry");
test.warning("does this show in sentry ?");
test.severe("some severe message!");  //this is only message that appeared in Sentry
test.fine("a fine message");
test.finer("a finer message");
test.finest("a finest message");
test.info("an info message");
test.shout("a shout message");

By default, the minimal log level which is recorded should be info. See https://github.com/getsentry/sentry-dart/blob/6b2a874e794068597bfe9778888d4cf3ed349069/logging/lib/src/logging_integration.dart#L17-L21 Did you configure it differently?

@ueman we might need to change the logic of comparing log levels, we compare the int value of the level right, but eg OFF has the value of 2000, so it'd log but its rather wrong, we should bail if it's OFF.

Just tried the new sentry_logger package. All installed fine etc.
When we tried some test log messages, the only one that made it through to Sentry was the "severe" category message. Is this by design ? Or do we need to set something up in Sentry to allow the different log categories to show up ?
var test = Logger("TestLoggerForSentry");
test.warning("does this show in sentry ?");
test.severe("some severe message!");  //this is only message that appeared in Sentry
test.fine("a fine message");
test.finer("a finer message");
test.finest("a finest message");
test.info("an info message");
test.shout("a shout message");

everything below severe is just a breadcrumb, so it won't be reported to sentry unless you capture a message or exception.
eg:
var test = Logger("TestLoggerForSentry");
test.warning("does this show in sentry ?");  // its a breadcrumb
test.fine("a fine message"); // wont be a breadcrumb, since its less than INFO
test.finer("a finer message"); // wont be a breadcrumb, since its less than INFO
test.finest("a finest message"); // wont be a breadcrumb, since its less than INFO
test.info("an info message");  // its a breadcrumb
test.shout("a shout message"); // it captures an error event
test.severe("some severe message!"); // it captures an error event
 Sentry.captureMessage('message'); // it captures a message event

What about test.info("an info message", error, stackTrace);?
Does this automatically create an event?

What about test.info("an info message", error, stackTrace);? Does this automatically create an event?

No, but it does log the exception and stacktrace in addition to the message as a breadcrumb.

@ueman we might need to change the logic of comparing log levels, we compare the int value of the level right, but eg OFF has the value of 2000, so it'd log but its rather wrong, we should bail if it's OFF.

@ueman should we create an issue for that? or does the logging library bail off automatically and the sentry integration will never be called?

I would say we create a ticket, because we can just close it, if it's not relevant :D

I would say we create a ticket, because we can just close it, if it's not relevant :D

https://github.com/getsentry/sentry-dart/issues/656

@sogogolf any news here?

Thanks for the follow up. Im a little confused by the thread above - is this considered a bug or is it behaving as intended ?

Thanks for the follow up. Im a little confused by the thread above - is this considered a bug or is it behaving as intended ?

The thread above is already solved thru https://github.com/getsentry/sentry-dart/issues/656 so please ignore it.
https://github.com/getsentry/sentry-dart/issues/654#issuecomment-988569402 and https://github.com/getsentry/sentry-dart/issues/654#issuecomment-988612554 require your input tho, thanks.

Closing the issue as a part of large repository cleanup, due to it being inactive and/or outdated. There's a large chance that this issue has been solved since.
Please do not hesitate to ping me if it is still relevant, and I will happily reopen and work on it.
Cheers!


Oversubscription with timed goals

Codecov Report
Patch coverage: 39.16% and project coverage change: -0.17 :warning:

Comparison is base (27c90ff) 89.66% compared to head (524d5b9) 89.50%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
- Coverage   89.66%   89.50%   -0.17%
==========================================
  Files         151      151
  Lines       19646    19713      +67
==========================================
+ Hits        17616    17644      +28
- Misses       2030     2069      +39

Impacted Files
Coverage Δ

unified_planning/test/examples/testing_variants.py
100.00% <ø> (ø)

...ed_planning/grpc/generated/unified_planning_pb2.py
62.11% <7.69%> (-0.04%)
:arrow_down:

unified_planning/grpc/proto_reader.py
86.63% <50.00%> (-1.06%)
:arrow_down:

unified_planning/grpc/proto_writer.py
84.78% <50.00%> (-1.13%)
:arrow_down:

unified_planning/model/metrics.py
82.92% <50.00%> (-5.97%)
:arrow_down:

unified_planning/io/python_writer.py
89.52% <54.54%> (-1.31%)
:arrow_down:

...ngines/compilers/disjunctive_conditions_remover.py
87.44% <57.14%> (-2.42%)
:arrow_down:

...ified_planning/engines/oversubscription_planner.py
87.96% <60.00%> (-2.14%)
:arrow_down:

...g/engines/compilers/negative_conditions_remover.py
95.87% <61.53%> (-2.49%)
:arrow_down:

..._planning/engines/compilers/quantifiers_remover.py
94.57% <64.28%> (-3.72%)
:arrow_down:

... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


kpt v1 cannot render from a directory without a Kptfile

Expected behavior
I should be able to run kpt fn render from a root directory with no Kptfile (but with child directories that potentially do have Kptfiles).
Actual behavior
Kpt refuses to render, raising this error:
Error: No Kptfile found at "/Users/morgantep/code/google/anthos/blueprints-gh/catalog/hierarchy".

Information
This is a regression from kpt 0.3x.

Thank you for reporting this. This is as per design, we need to document this better in our docs.
fn render is a bit different from the fn run command in 0.3x when it comes to rendering the config.  fn render renders a package hierarchy in depth-first-order where resources from sub-package are render first, and then resources from subpackage rendering are re-rendered at the parent with parent package's pipeline. Declarative Function Execution chapter covers the fn render in detail.
render detects package boundary by the presence of valid Kptfile, otherwise, it will not be able to differentiate between a directory without kptfile and directories with kptfile. Consider following examples of package hierarchy.
# Example 1
root-pkg
   Kptfile
   subdirA
     Kptfile
     r1
     r2

# Example 2
root-pkg
   Kptfile
   subdirA
     r1
     r2

In the example1, r1 and r2 belongs to subdirA subpackage, while in example2, r1 and r2 belongs to root-pkg. So having a strict notion of a package is important for render to determine the package hierarchy and apply functions.
If you are running functions imperatively, you will find fn eval behavior closer to fn run in 0.3x.
Is catalog/hierarchy a kpt package or not ? if it's a package, you should initialize Kptfile using kpt pkg init to run kpt fn render on it.

/cc @frankfarzan @phanimarupaka

Are you saying we don't want to run kpt fn render in any sort of CI/CD workflow?
In the landing zone, we have a "source repo" which contains the unhydrated config of what we want to have deployed. It looks like this:
source-repo/
-- package-one/
---- Kptfile
---- resources.yaml
-- package-two/
---- Kptfile
---- subpackage/
------ Kptfile

In my Cloud Build set up, I want to render all packages in the repo (so Config Sync can then apply them to the cluster).
I am not asking to run imperative functions. This is simply asking to declaratively render all functions.
This is a major regression from kpt 0.3x and needs to be addressed before launch.

In your example above, you are actually treating source-repo as a package that has two subpackages. To invoke render on the source-repo package, just create a minimal Kptfile. The following is a porcelain for this:
$ kpt pkg init source-repo

Definition of package:
A package is explicitly declared using a file named Kptfile containing a KRM resource of kind Kptfile. The Kptfile contains metadata about the package and is just a regular resource in the YAML format.
https://kpt.dev/book/02-concepts/01-packages
Render is one of the operations that can be done on the noun package, there are other operations that are defined on this noun and require Kptfile by definition:
https://kpt.dev/book/02-concepts/02-workflows
So this is not artificial constraint or specific to render, it's how a package is defined consistently across all functionality in kpt. And most other package managers have similar definition.
===================
Side note:
An alternative way to define a package would be to say each directory is a package (1-to-1 mapping), a package (i.e. a directory) may have zero or one Kptfile. This was briefly considered in early designs on but not chosen for several reasons.

Hi @frankfarzan, thanks for weighing in. I agree with your assessment that source-repo is implicitly a package with two subpackages.
What I don't understand is why we need to force users to manually create a Kptfile just to run kpt fn render. If a content-less kpt package is sufficient to run kpt render, why do we force the toil of generating that package on users?
Respectfully, it is an artificial constraint if we force users to take a manual action which adds no new information to the system.

Side note: An alternative way to define a package would be to say each directory is a package (1-to-1 mapping) and a package (i.e. a directory) may have zero or one Kptfile. This was briefly considered in early designs but not chosen for several reasons.

I was not aware we changed the definition from 0.39. Can you please share the design doc where it was approved to decide that all packages must have a Kptfile?

Take @droot's example above, how would you know whether root-pkg/subdirA is a package? Should you be able to run  a command like live apply in that directory for Example 2?
In Example 2, running render or any other package operation (e.g. live apply) on that directory needs to produce a clear error message instead of silently trying to do the wrong thing. Accepting any directory as a package will lead to all sorts of foot-guts and inconsistencies like this. This is where providing consistent and simple definition of what a package is across all commands is more beneficial than small convenience of this implicit behavior for the render command. It's also much easier concept to explain to users.
The alternative model I described above was never implemented in kpt, only briefly considered. @mortent can provide more history here.

Take @droot's example above, how would you know whether root-pkg/subdirA is a package? Should you be able to run a command like live apply in that directory for Example 2?

If a user decides to run from that directory, they should be able to do so.

The alternative model I described above was never implemented in kpt, only briefly considered. @mortent can provide more history here.

In kpt 0.39, I could run kpt pkg get and kpt fn run without a Kptfile.

So in your Cloud Build, you're doing this:
$ kpt pkg get <git-repo>/source-repo
$ kpt fn render source-repo

?
If so, note the following:

There is one scenario where a Kptfile is implicit: You can use kpt to fetch any Git directory containing KRM resources, even if it does not contain a Kptfile. Effectively, you are telling kpt to treat that Git directory as a package. kpt automatically creates the Kptfile on the local filesystem to keep track of the upstream repo. This means that kpt is compatible with large corpus of existing Kubernetes configuration stored on Git today! For example, cockroachdb is just a vanilla directory of KRM: $ kpt pkg get https://github.com/kubernetes/examples/staging/cockroachdb

https://kpt.dev/book/02-concepts/01-packages
On the local filesystem, the minimal Kptfile is automatically created. You can potentially take advantage of that feature, even though in your use case, you know you're dealing with kpt packages (and have declared sub-packages), so not sure what you're really gaining by avoiding making source-repo an explicit package.

Cloud Build already provides the Git repo, so I'm not running kpt pkg get.
Obviously I can work around this by inserting an auto-generated kpt pkg init, but it still seems suboptimal.

@morgante Some different options for what is considered a package and how to demarcate package boundaries were considered. In the end, we decided that defining a package by the presence of the Kptfile is the best option. Almost every kpt package requires the Kptfile to be useful (like the apply logic, update, diff..) and it allows us to create a UX that is consistently package-focused. The new kpt package design also avoids the different treatment of a root package vs a subpackage from previous versions of kpt, which extends to the requirement that packages must have a Kptfile.
How would you end up with a root package that doesn't have a Kptfile? If you are creating the package from scratch, it seems like you would want to start with creating the Kptfile. And if the package is cloned from a git repo, kpt will create the Kptfile automatically.

Obviously I can work around this by inserting an auto-generated kpt pkg init, but it still seems suboptimal.

Thinking from a package publisher's CUJ perspective, I am ready to create a new package which will contain other sub packages, doing the first step of initializing the package and declaring the intent explicitly with kpt pkg init seems very reasonable from UX perspective given that this needs to be done once in package's lifecycle.

Thinking from a package publisher's CUJ perspective, I am ready to create a new package which will contain other sub packages, doing the first step of initializing the package and declaring the intent explicitly with kpt pkg init seems very reasonable from UX perspective given that this needs to be done once in package's lifecycle.

That's fine, I have no concerns with doing that for package publishers.

We don't plan on supporting this, so closing this.


Update various typos, punctuations & errors.

Thanks for the PR @preserveddarnell . Reached out about appreciation via email already.


[NOT FOR MERGE] Devel next

@0xDen
I'm not sure what was the purpose of this branch. It was not part of any MR and it was not referenced anywhere. Please verify if commits from here are already included elsewhere. Do we need this branch, or I can get rind of it?

@macpijan  this branch has been added to continue work while the devel branch has unmerged PRs. If our workflow is to create a new git branch for each new feature and then merge it to main branch then Okay, this branch could be removed.

The branch devel-next will be removed


Issues with crafting archwood drawers

All the archwood drawers have the same crafting recipe. This makes it difficult to craft the color you want without a mod like Polymorph. Maybe make like a base grey one then add the log of the one you want?

i will be changing that.


Add LDAP search UI

Coverage increased (+0.006%) to 64.537% when pulling aeda641fc37bb4c0c40147552398babba0a6a680 on ninjadq:ldap_search_ui into a0bf7196e63f67c64df24bb6455841772abca4a1 on vmware:master.

Please fulfill your PR description first


Security Warning :: Your database may be open to everyone

Once you have connected to the database, you can control your database by opening the page in any other browser.
https://www.shodan.io/search?query=pgweb

Please change your docker run command to
docker run -e SESSIONS=true


[Bug] No-RlsGroup is too agressive.

Which Guide
Radarr
Describe the Error in the Guide
The "No-RlsGroup" format will often exclude releases from indexers who use spaces in their formatting.
Expected behavior
Keep these releases.
Screenshots

Same releases, but one is marked as not having a release group.

not a guide issue; those do not have a group.
https://github.com/Prowlarr/Indexers/issues/156


IPAM Overhaul: IPAddress SLAAC, DHCP as types

Currently IPAddress still relies on the Status of SLAAC cached on the model as IPAddress.STATUS_SLAAC, which should be removed.
Additionally, DHCP is also a status that should be converted and removed as a status.
These should be replaced with a type enum field similar to Prefix.type (See: #1362).
Care needs to be taken in the approach here. For example, SLAAC is an IPv6 only option. DHCP arguably could be used for either IPv4 or IPv6 depending on implementation details.
### Acceptance Criteria
- [x] Remove `IPAddress.STATUS_SLAAC` and any associated business logic
- [x] Remove `SLAAC` `Status` object and `IPAddressStatusChoices.STATUS_SLAAC`
- [x] Remove `DHCP` `Status` object `IPAddressStatusChoices.STATUS_DHCP`
- [x] Implement a  new `IPAddressTypeChoices` enum to reflect the available `IPAddress` types
- [x] Assert that `SLAAC` can only be used for IPv6 addresses

Closed with #3874


Girderify

The mapping interface appears to be working—if you guys get a minute, could I get you to try it?

This is looking very good code-wise and when I look at the application. I think there are some UI simplification steps to think about (I had to click around before I started to get the hang of it), but I'm impressed how many things work, even going back and changing the data, expanding all the panels, etc. I don't think this PR is supposed to address all UI issues, so LGTM.
I see that the toolchain has multiple datasets and multiple visualizations (they are arrays). Since we will be doing single-data, single-vis toolchains for a while, I don't want support for multiple data/vis to add too much complication, but I don't think it does so it's ok to leave it in.

Yeah, it's definitely not super polished yet, but I think it's almost at the walking skeleton level. I mainly left the arrays (multiple datasets + multiple visualizations) in so that we wouldn't have to completely rewrite the mapping interface again when we get there... for now, it's pretty trivial to just grab the first element.

Other than #26, I think the rest of the changes are mostly aesthetic at this point.
I totally agree with #29 --- I need to do a redesign anyway to fit the look and feel of the app a little better, and I can address the usability cues along the way. The critical bits are there now... per your earlier LGTM I'll go ahead and merge.

@alex-r-bigelow thanks for all the changes! In the future, a LGTM only applies to the current state of the PR. If you push some commits, you'll need another LGTM. In this case, I needed to go back and verify that you resolved each of the issues that you fixed on the rest of the branch, but that should have been done before a merge. I was assuming the issues I opened would be resolved in future PRs. No biggie.

Oops, sorry about that! Still kind of new to doing real collaborative stuff on github...


Changed version request credentials to use clientContext credentials

Minor update to have the SP Version Request use the clientContext.Credentials instead of the default.
This addresses an issue where authentication sometimes fails to on-premises for cross-site transformation and causes the connection to be classified as SPO (which causes failures).

Thanks @thechriskent :-)


feat(kit): use pages from the module without setting the pages option to true

🔗 Linked issue

resolves https://github.com/nuxt/nuxt/issues/20801
❓ Type of change

[x] 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
[ ] 🐞 Bug fix (a non-breaking change that fixes an issue)
[x] 👌 Enhancement (improving an existing functionality like performance)
[ ] ✨ New feature (a non-breaking change that adds functionality)
[ ] 🧹 Chore (updates to the build process or auxiliary tools and libraries)
[ ] ⚠ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Sets pages option to true if extendPages is called.
If extendPages gets called, it means that there are actually some pages, so we can set te pages option to true?
+ updates typo in documentation
📝 Checklist

[x] I have linked an issue or discussion.
[x] I have updated the documentation accordingly.

Thank you for your fast work, but I think there's one edge case left behind which is not my case, but may happen to others later.
You can also extend pages by using hook - pages:extend instead of extendPages() function.
Shouldn't it be better to set it after await nuxt.callHook('pages:extend', pages) here ? If pages array after hook call is not empty, then surely we can set nuxt.options.pages = true; right?

The safest thing is probably to do this within isPagesEnabled, directly before returning false:
const pages = await resolvePagesRoutes()
await nuxt.callHook('pages:extend', pages)
if (pages.length) {
  return true
}

We can keep the previous early returns for true as a more performant fast-path.

I agree, but that means that the hook will be called multiple times if there are no pages, so I wasn't sure if that's a good idea.


Feature request: let pypi query the latest release string from the repository homepage to determine whether the package is latest or not

Describe the bug
https://pypi.org/project/binwalk/ reports Latest Version, whereas the latest version is 2.3.4 (
I'm not sure if this is manually set, or automatically. If the latter, then this is a bug.
Expected behavior
Should show that this is not the latest version
To Reproduce
Go to the page
My Platform
N/A

Hi, this issue tracker is for issues with the codebase behind https://pypi.org itself, not the projects hosted on PyPI. This project has only make the single release listed to PyPI.
You should report this issue to the tracker for the project in question instead.

@di I know that, as I said at the beginning, manual or automatic.
So, what prevents pypi to automatically determine whether the project is up to date or not? It could use github's API.

@dgutson I think an important distinction is that PyPI is not equivalent to a project's source - which could be GitHub, it could be anything - there's no requirement for these to be connected.
PyPI's notion of "latest" is the most recently-released package on the platform - and if the maintainers of the project have not elected to publish, that's entirely their prerogative.
In essence, PyPI is behaving 100% correctly - the latest version is indeed the latest version we have, which is why you should communicate the lack of a newer release to the project maintainers.

https://github.com/ReFirmLabs/binwalk/issues/523 seems to be the relevant issue

@miketheman I perfectly understand what PyPI is and its relationship with github or others (gitlab, bitbuket, etc).
I'll turn this a feature request: let PyPI connect to different source providers (based on the homepage field) and determine whether the package in PyPI is the latest build or not.
@di thanks, binwalk was just an example.

@miketheman would you consider pls to re-open now that it is re-scoped and I changed the title?

@dgutson Considering the description and conversation, I'd recommend opening a fresh feature request issue with the specifics.
I will point out that there's already complexity in "trusting" the repository metadata since it's not verified.
See #8462 #10917 for some previous issues on this topic.


fix(compiler): use Program as fallback for language service

Related to #1336

Pull Request Test Coverage Report for Build 3984

19 of 20 (95.0%)  changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 90.625%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

src/compiler.ts
19
20
95.0%

Totals

Change from base Build 3972:
0.1%

Covered Lines:
1083

Relevant Lines:
1139

💛  - Coveralls


Can't See nor use the emote menu.

i tried installing this:
https://cdn.discordapp.com/attachments/119918741516451840/345554778832633867/emotes.zip
but to no avail, looked around but couldnt find anything, really sick and tired of scrolling through the tiny, default emoji menu in discord.

Same here. The Twitch emotes work. The menu just doesn't show up.
Changing the option in the settings does nothing, and turning off all my plugins doesn't help either.

Using BandagedBD seems to fix it. Also made some of my plugins work again.

thank you man, you fixed it! one more thing, i cant seem to be able to favourite non twitch emotes?


GPG Public key location

Question
Where can I get the public gpg key that is used to sign the packages in maven? The key used to sign the jar is B2005A1A778C404F37E43B2401B0B092A6925976 but I haven't found it on any keyservers and I can't find any documentation on the internet about it.

This would be really important because the current JDBC driver versions (11.2.1) cannot be verified when we build our product with maven or gradle.


Lint project

Adds prettier and lints the project

Whoops, the revert commit also adds in the format script.


extension for a specific string function: reverse()

https://github.com/mayflower/sqlite-reverse-string was useful to me once, it has no dependencies, but could be obsoleted by a library encompassing more string functions

It's already implemented in math extension.

provide mathematical and string extension functions for SQL queries using the loadable extensions mechanism. Math: acos, asin, atan, atn2, atan2, acosh, asinh, atanh, difference, degrees, radians, cos, sin, tan, cot, cosh, sinh, tanh, coth, exp, log, log10, power, sign, sqrt, square, ceil, floor, pi. String: replicate, charindex, leftstr, rightstr, ltrim, rtrim, trim, replace, reverse, proper, padl, padr, padc, strfilter. Aggregate: stdev, variance, mode, median, lower_quartile, upper_quartile.

It's not. math extension contains only math functions. I don't know the source of your quote, but it's not the docs.

https://www.sqlite.org/contrib
https://www.sqlite.org/contrib/download/extension-functions.c?get=25

This is an old third-party extension, which has nothing to do with math extension.

Yeah, I missed that you are using another math extension.

@nalgeon sounds like that reverse would actually be useful to have in the unicode extension since that is dealing with strings in general.

I am working on the text extension, reverse() will be there.

Done.

thank you! I was pleasantly suprised you included split_part (substring_index for mysqlers) too, an often used function


Documentation lacks information about "DNSSEC Enable" feature

Change Type
Addition
Area
Configuration
Proposed Changes
Currently It is possible to enable DNSSEC from UI but rest of actions not clear, I know it is required to add some DS records to domain registerer and know How to get required data from server but is it also possible to get those information from UI? imagine there is a non admin user and he wants to enable DNSSEC for domain he controls, after enabling DNSSEC on zone how he can get required DS information to be published on domain registerer?

Hi just look into the PowerDNS Doc: https://doc.powerdns.com/authoritative/dnssec/pdnsutil.html
Generate Keys:
pdnsutil secure-zone yourdomain.com
View Keys:
pdnsutil show-zone yourdomain.com
Then add the keys to your registrar.

@whitepiratebaku it is not within the scope or responsibility of this project to teach users about DNS fundamentals. Please use search engines if you wish to learn more.


[Suggestion] Twitch / YouTube Live / LiveStream / etc support

This app is SO much easier to use than OBS for recording from the screen.
Is there any way you'd consider adding Twitch / YouTube Live / LiveStream / etc support for real time streaming?

Have not tried but will give it a shot.
Some Links to check

https://trac.ffmpeg.org/wiki/EncodingForStreamingSites
https://gist.github.com/olasd/9841772

FFmpeg's site made it look like you just had to pass a specific command line to FFmpeg  https://trac.ffmpeg.org/wiki/StreamingGuide , but I don't know how difficult this would be to work into what you already have in Captura.
  Presumably you could just prompt users on how to get this key and save it for use every time they stream.

I followed the guides and tried streaming to both Twitch and YouTube Live.

I haven't succeed with YouTube Live 😢
Streaming to Twitch worked but there was a delay of approximately 10s.

Maybe the problem is that my internet connection is not fast enough.
Will try some more workarounds.

It's not unusual to have some latency on Twitch.   As you linked to in your encoding guide, make sure you use -crf when encoding for streaming.
I found this .sh script in a gist for YouTube Live streaming via FFMPEG, hopefully it's helpful: https://gist.github.com/olasd/9841772

I have tried that script already. Yes, I'm using the -crf option.
I'll test on a better internet connection in a few days and let you know.

Throw up a build and I'll help you test

https://ci.appveyor.com/api/buildjobs/0cp18jiabrb2v04r/artifacts/temp%2FCaptura-Portable.zip
Go to Configure > FFMpeg and set up Twitch and YouTube Live keys.
Select Streaming as video writer type and select Twitch or YouTube Live.
Thank You

I'm able to reproduce both of your results.
Twitch.tv works with 9-10 second latency (which is similar to what I get on OBS or Geforce Experience) streaming.
Youtube Live says "Starting" for status, but apparently never receives data because even after 1 minute of streaming nothing ever appears.  I tried a few different options like "-i 0.0" and "-strict experimental" in arguments but wasn't able to get it to work no matter what I tried.

To debug things I tried setting to a local rtmp address, i.e. rtmp://<IP_ADDRESS>:8090. However, ffmpeg crashes right around 6 seconds into the recording with the following error (this is with captura 9.0.0 beta):
ARGS:
-------------
-thread_queue_size 512 -framerate 20 -f rawvideo -pix_fmt nv12 -video_size 2560x1440 -i \\.\pipe\captura-95b0d585-c58c-4f29-be04-cf3790ba6f18 -r 20 -vcodec libx264 -crf 15 -pix_fmt yuv420p -preset ultrafast -g 40 -r 20 -f flv rtmp://<IP_ADDRESS>:8090/stream

OUTPUT:
-------------
ffmpeg version git-2020-03-30-8d019db Copyright (c) 2000-2020 the FFmpeg developers
  built with gcc 9.3.1 (GCC) 20200328
  configuration: --enable-gpl --enable-version3 --enable-sdl2 --enable-fontconfig --enable-gnutls --enable-iconv --enable-libass --enable-libdav1d --enable-libbluray --enable-libfreetype --enable-libmp3lame --enable-libopencore-amrnb --enable-libopencore-amrwb --enable-libopenjpeg --enable-libopus --enable-libshine --enable-libsnappy --enable-libsoxr --enable-libsrt --enable-libtheora --enable-libtwolame --enable-libvpx --enable-libwavpack --enable-libwebp --enable-libx264 --enable-libx265 --enable-libxml2 --enable-libzimg --enable-lzma --enable-zlib --enable-gmp --enable-libvidstab --enable-libvmaf --enable-libvorbis --enable-libvo-amrwbenc --enable-libmysofa --enable-libspeex --enable-libxvid --enable-libaom --enable-libmfx --enable-ffnvcodec --enable-cuda-llvm --enable-cuvid --enable-d3d11va --enable-nvenc --enable-nvdec --enable-dxva2 --enable-avisynth --enable-libopenmpt --enable-amf
  libavutil      56. 42.102 / 56. 42.102
  libavcodec     58. 77.101 / 58. 77.101
  libavformat    58. 42.100 / 58. 42.100
  libavdevice    58.  9.103 / 58.  9.103
  libavfilter     7. 77.101 /  7. 77.101
  libswscale      5.  6.101 /  5.  6.101
  libswresample   3.  6.100 /  3.  6.100
  libpostproc    55.  6.100 / 55.  6.100
Input #0, rawvideo, from '\\.\pipe\captura-95b0d585-c58c-4f29-be04-cf3790ba6f18':
  Duration: N/A, start: 0.000000, bitrate: 884736 kb/s
    Stream #0:0: Video: rawvideo (NV12 / 0x3231564E), nv12, 2560x1440, 884736 kb/s, 20 tbr, 20 tbn, 20 tbc
Multiple -r options specified for stream 0, only the last option '-r 20' will be used.
[tcp @ 00000113feda1240] Connection to tcp://<IP_ADDRESS>:8090 failed: Error number -138 occurred
[rtmp @ 00000113feda0d00] Cannot open connection tcp://<IP_ADDRESS>:8090
rtmp://<IP_ADDRESS>:8090/stream: Unknown error

I'm not sure if anything is streamed at all.
I'm investigating whether it's just a bad set of ffmpeg args. But conceptually this is all happening on the ffmpeg side, and the only change is the change in destination (from a local file to a rtmp address), so I'm a bit confused as to why it crashes.

@ericjang How are you creating the RTMP server at rtmp://<IP_ADDRESS>:8090? Are you using FFserver?


AEM Advanced Permissions Manager - Core (aapm.core) in Installed state in AEM 6.5.13

On installing the 'all' package in AEM 6.5 sp 12 and sp 13, the AEM Advanced Permissions Manager - Core (aapm.core) bundle is in Installed stage.
As it is not able to resolve org.apache.jackrabbit.oak.spi.security.authorization.restriction,version=[1.2,2)

Other issues:

AC Tool is different for cloud/non-cloud (we need a separate cloud-examples package)
Subpackages are defined differently for coud/non-cloud (embed vs. subpackage)


add markup for emphasized phrases

many paragraphs begin with a topic sentence that has bolded text. these need to be identified with markup for semantics and to match the style of the source doc

fixed in #91 ; file can be viewed at http://open-data-manual.readthedocs.io/en/latest/directive-2013.html


Fast datapath

Smoke tests pass (modulo the one that is broken on master), so I think this is fit for a review pass.
Things still to do:

The weave script changes are still a bit grotty.  But...
I think there needs to be a "disable it all" option to weave, which disables fastdp, --net=host, and the MTU change.
I haven't modified architecture.txt. To be honest, most of the contents are more like implementation notes that belong as comments in the code.  If the fast datapath code requires additional notes of that nature, I'd be happy to add them as comments. A high-level overview of fast datapath would be a good thing, but I think I'll need to re-write the whole of architecture.txt to match.
1.1 compatibility support.  That will be a separate issue.  Short ids is the tricky part here.
I need to check out a kernel oops I saw when testing with ubuntu 12.04 a few days ago.

Regarding the create/delete datapath & add datapath interface commands added to weaver:

Previously, the proxy would run the weave script in ways that did not run the weaver executable.  Now they can.

We've started (for better or worse) to develop a stable of little helper executables under prog/* - might these functions be moved there? If they were, would it ameliorate the proxy coverage issue?

We've started (for better or worse) to develop a stable of little helper executables under prog/* - might these functions be moved there? If they were, would it ameliorate the proxy coverage issue?

Where there is no functional overlap, I think having a separate helper program is natural.  That's not the case here.  And we probably do want coverage on those helper programs, too.

I have a couple of questions regarding the short peer ID allocation code:

Rather than the two fields in ShortIDPeers, could you maintain a single sorted slice holding the peers in order of ascending name? You might save some complexity in addByShortID/deleteByShortID by leveraging the sort package, as well as reducing the amount of test code required to check the invariants associated with the existing peer/others design. (Initially I wondered if you could use container/heap for this but you need to be able to remove peers by name)
At the cost of pre-allocating 4096 ShortIDPeers structs and storing them in a free entries map, you could drastically simplify chooseShortID; even more so if you rely on Go's map iteration randomisation:

func (peers *Peers) chooseShortID() (PeerShortID, bool) {
    for shortID, _ := range peers.freeShortIDs {
        return shortID, true
    }
    return 0, false
}

add/delete then move entries between the free/in-use maps; hopefully this would make the mechanism more 'obviously correct' and obviate the need for the complex unit tests.

Things still to do

Add smoke tests for FDP?

Add smoke tests for FDP?

Such as?  The existing smoke tests exercise FDP.  What functional additions are there to test?

What functional additions are there to test?

Fallback behaviour.

What functional additions are there to test?

Fallback behaviour.

Already there: test/105_frag_2_test.sh

What functional additions are there to test?

Fallback behaviour.

Already there: test/105_frag_2_test.sh

You're going to have to help me out - that test isn't modified in the PR, and I can't see how the current version tests fallback. Here's what I think will happen:

Both forwarders will race to establish. They will both succeed, but because fdp is preferred it will end up as the 'best' forwarder and the overlay forwarder will be stopped (probably before it completes PMTU discovery)
The large ping will get fragmented according to the overlay MTU, and as the VXLAN path is unaffected by the iptables rule it will succeed

What am I missing?

What am I missing?

The test introduces a iptables rule to discard packets > 700 bytes.  fastdp sends a heartbeat packet sized to the overlay MTU (i.e. 1410 bytes).  This won't get through, and so fastdp will never report that it is established (and ultimately it will time out and report an error).  But sleeve will report that it is established, and so be selected as the active overlay.  The test then tests sleeve fragmentation behaviour as before.

fastdp sends a heartbeat packet sized to the overlay MTU

Argh I did check for that but I took the comment
 587     // the heartbeat payload consists of the 64-bit connection uid
 588     // followed by the 16-bit packet size.

literally and missed the +fwd.fastdp.mtu in the buffer allocation :smile:
Thanks!

I need to check out a kernel oops I saw when testing with ubuntu 12.04 a few days ago.

Now fixed.

@dpw I've a question about OVS behaviour. If I have three nodes in a full mesh, and I send an ethernet broadcast from one of them I end up with a flow like this (broken across lines for clarity):
weave
--in-port=vethwepl13226
--eth-src=4a:81:be:6b:45:2d --eth-dst=ff:ff:ff:ff:ff:ff
--set-tunnel-id=0000000000ca2614 --set-tunnel-ipv4-src=<IP_ADDRESS> --set-tunnel-ipv4-dst=<IP_ADDRESS> --set-tunnel-ttl=64 --set-tunnel-df=true
--set-tunnel-id=0000000000217614 --set-tunnel-ipv4-src=<IP_ADDRESS> --set-tunnel-ipv4-dst=<IP_ADDRESS> --set-tunnel-ttl=64 --set-tunnel-df=true
--output=vxlan-6784,vxlan-6784,weave

Here there are two set-tunnel actions, one for each peer needing to receive the broadcast, and the vxlan vport appears twice in the output action. How is it that the first output to the vxlan vport is associated with the first set of tunnel parameters, and the second output with the second set? A naive interpretation suggests that the first set of tunnel parameters are overwritten by the second set before we reach the output action, and we then output the packet twice to the same peer...

How is it that the first output to the vxlan vport is associated with the first set of tunnel parameters, and the second output with the second set?

Ahh, it would seem this is an issue with odp flow list weave collecting output actions and displaying them out of sequence at the end of the action list...

Ahh, it would seem this is an issue with odp flow list weave collecting output actions and displaying them out of sequence at the end of the action list...

Yes, exactly.
--output actions were represented that way due to the limitations of the golang flags module.  But it is silly, and other actions are not aggregated like that, which means that you couldn't use the tool to set them.
It's a mess, bu-t the tool was meant for experimenting with OVS, and it's sometimes useful for debugging.  But I wouldn't lean on it too heavily.
If you want to know what the flows created by weave look like, run with --log-level=debug. The resulting output is ugly, but (intended to be) accurate.

Assume a peer A listening on our standard vxlan port is instructed to connect to another peer B running on a non-standard vxlan port:
A (6784) ----------------------> B (9001)

In this case, due to the if remoteAddr != nil conditional in MakeForwarder, the originating peer A creates a new vxlan vport for port 9001 primarily so that it can send vxlan encapsulated packets to B.
On the other hand B, not taking the conditional, selects its own main vxlan vport on 9001 for forwarding to A - this only works because a side effect of A creating a vxlan vport for 9001 is that it also begins listening for vxlan traffic on this port.
Have I understood this correctly? If so, I have some further thoughts:

The behaviour is quite subtle and could probably stand a comment in the code as to why the receiving peer doesn't need to allocate a vxlan vport when it eventually 'learns' the remote address
As it stands, B never actually learns that As main vxlan port is 6784 because the remote port passed to handleVxlanSpecialPacket is actually the UDP port used to create Bs own main vxlan vport in getVxlanVportID
The behaviour is counter-intuitive from a users perspective - they would expect vxlan traffic from B->A to be destined for port 6784. They may even have firewall rules that enforce this expectation, preventing B from forwarding to A

The behaviour is quite subtle and could probably stand a comment in the code as to why the receiving peer doesn't need to allocate a vxlan vport when it eventually 'learns' the remote address

Ok.

As it stands, B never actually learns that As main vxlan port is 6784 because the remote port passed to handleVxlanSpecialPacket is actually the UDP port used to create Bs own main vxlan vport in getVxlanVportID

Yes. There is no way for it to learn that. The source port on the vxlan packets is not 6784.

The behaviour is counter-intuitive from a users perspective - they would expect vxlan traffic from B->A to be destined for port 6784. They may even have firewall rules that enforce this expectation, preventing B from forwarding to A

Then we'll fall back to sleeve.

If you want to know what the flows created by weave look like, run with --log-level=debug. The resulting output is ugly, but (intended to be) accurate.

I'd really like to see some additions to weave status:
weave status connections to show which forwarder is in use
weave status odp display current state of flows
it would be nice to get these into 1.2 for support purposes, but I don't want to hold up this PR for that - since the review is drawing to a close, how about I create an issue for it and work on it myself whilst you're tidying up the last bits and pieces? @dpw @rade

weave status connections to show which forwarder is in use

That seems useful.

weave status odp display current state of flows

Perhaps this should just go into weave report.

BTW my WIP 'how it works' doc is here.

That seems useful.

#1505.

For the avoidance of doubt, other than giving the remaining loose ends the once over I am ready to merge this.

I have a couple of thoughts regarding the short peer ID allocation code:
Rather than the two fields in ShortIDPeers, could you maintain a single sorted slice holding the peers in order of ascending name? You might save some complexity in addByShortID/deleteByShortID by leveraging the sort package, as well as reducing the amount of test code required to check the invariants associated with the existing peer/others design. (Initially I wondered if you could use container/heap for this but you need to be able to remove peers by name)

It's true that it would be slightly simpler (the simplifications would mainly occur in deleteByShortID).  But I don't think it would make a huge amount of code go away.  We would still need to detect cases where invalidateShortIDs must be set.  Similarly for the tests: Ok, they have an added importance in the context of the code as it is. But even with your proposal, I don't think the amount of testing code would be significantly reduced, due to the need to exercise corner cases that would still exist.
Set against that, it seems an over-specified solution to me, and would be less efficient (not in any way that is ever likely to matter, I admit).  If golang was a nice high-level language, I would worry more about that code not being succinct.  Golang is not a nice high-level language.

At the cost of pre-allocating 4096 ShortIDPeers structs and storing them in a free entries map, you could drastically simplify chooseShortID; even more so if you rely on Go's map iteration randomisation:

I consider relying on golang's randomisation of the iteration start point to be an anti-pattern on a par with relying on map iteration to be stable in a language that does not guarantee that.

I think I've addressed everything that hasn't otherwise been responded to. There's a squashed-and-rebased branch at https://github.com/weaveworks/weave/tree/fdp-rebase , though I'll push that as fdp for this PR when everyone is happy.

There was a comment about defining const PeerShortIDMask = 1<<PeerShortIDBits - 1 in peer.go and using that in randomPeerShortID/extractPeers. Also extractPeers and tunnelIDFor don't make use of PeertShortIDBits as they should
We were going to have a paranthetical (e.g. sleeve forwarder) in send()`

There was a comment about defining const PeerShortIDMask = 1<<PeerShortIDBits - 1 in peer.go and using that in randomPeerShortID/extractPeers. Also extractPeers and tunnelIDFor don't make use of PeertShortIDBits as they should

Now you mention it, I do recall that discussion.  But I don't see it anywhere on this PR, which seems odd (and I have been careful not to do any rebasing on the fdp branch during this review).  I dug out the relevant github notification email, and the link in it no longer works.  That is rather disappointing.
I also recall that I responded to that suggestion, though maybe you weren't convinced by my response.  Do you have the email for that response?  My point was that the values in extractPeers/tunnelIDFor represent a definition of the encapsulation format, and are not necessarily the same as PeerShortIDBits.  That would leave only a single potential use of PeerShortIDMask.

We were going to have a paranthetical '(e.g. sleeve forwarder)' in send()

I did overlook this one when going back through all the comments.  Fixed now.

Found it: https://github.com/weaveworks/weave/pull/1438#discussion_r40575163 - I didn't notice it before, sorry! Accepting that the literal 12s in fastdp.go are only coincidentally equal in value to PeerShortIDBits, I might ask why they do not merit their own separate constant from which 0xfff is derived, but now that I look we seem to have lots of magic literal values sprinkled elsewhere so I suppose it doesn't make a lot of difference :smile:

Found it: #1438 (comment) - I didn't notice it before, sorry!

Thanks. So github has it associated with an "outdated diff", which is why I couldn't find it. But why does github think it is outdated?  There's been no rebasing or other history rewriting.  So I remain puzzled.

Accepting that the literal 12s in fastdp.go are only coincidentally equal in value to PeerShortIDBits, I might ask why they do not merit their own separate constant from which 0xfff is derived, but now that I look we seem to have lots of magic literal values sprinkled elsewhere so I suppose it doesn't make a lot of difference :smile:

Yes.  I believe symbolic constants have their place, but many people use them in contexts where I consider it unnecessary.

But why does github think it is outdated? There's been no rebasing or other history rewriting.

Oh, it regards any region touched by a later commit on the branch to be outdated.  I wish there was an "unhide all" button.
Ok, I've found another unresolved item I missed.  I'll have to do another sweep...

Ok, I've found another unresolved item I missed. I'll have to do another sweep...

No, I had addressed it.  Right, I think that's it.

There's a comment in the weave script that mentions a --no-fastdp option.

I think we are there - go ahead and push fdp-rebase->fdp. What did you want to do about the documentation and weave status PRs that I have open against this branch? That was just for receiving comments more than anything else, they can easily be reopened against master once this PR is merged if you'd prefer.

I think we are there - go ahead and push fdp-rebase->fdp.

Done

What did you want to do about the documentation and weave status PRs that I have open against this branch? That was just for receiving comments more than anything else, they can easily be reopened against master once this PR is merged if you'd prefer.

Yes, I think they will have to be rebased against master (or the new fdp branch).
I'm happy to look at the weave status one if @rade doesn't want to (I looked at part of it already).  It would be good to get it merged today.
I will look at the documentation one, sorry I haven't given it any attention so far.  But that doesn't seem like a blocker for the 1.2 release?

I will look at the documentation one, sorry I haven't given it any attention so far. But that doesn't seem like a blocker for the 1.2 release?

What piece of documentation do you suggest we link to in the release note item? Also, I would appreciate suggestions on how to describe the FDP feature in the release notes.

I consider relying on golang's randomisation of the iteration start point to be an anti-pattern on a par with relying on map iteration to be stable in a language that does not guarantee that.

I guess you're not a fan of Routes.RandomNeighbours() and GossipChannel.gcSenders/relayBroadcast() then :smile:

why is the reported coverage so poor?

What piece of documentation do you suggest we link to in the release note item?

Perhaps now that we have two overlays, there should be a section in features.md that discusses the encryption/speed tradeoff, port requirements and WEAVE_NO_FASTDP?

why is the reported coverage so poor?

Most of that seems to be due to go-odp. Why is that package showing up in the weave coverage reports?

What piece of documentation do you suggest we link to in the release note item?

That's a good question, but I don't think Adam's doc can be the answer to that.

Also, I would appreciate suggestions on how to describe the FDP feature in the release notes.

Ok.  Do you have an opinion on how much should we say in the release notes, and how much in the docs?  I would tend towards saying anything substantial in the docs, and then the release notes can just mention the presence of the feature, and say that it is possible to do a rolling upgrade from 1.1 to 1.2 without special measures.

Most of that seems to be due to go-odp. Why is that package showing up in the weave coverage reports?

Shrug.  I don't know what would cause it to be treated differently from any other dependency.

I don't know what would cause it to be treated differently from any other dependency.

Perhaps it's because they're both under the weaveworks github org?

Perhaps it's because they're both under the weaveworks github org?

https://github.com/weaveworks/weave/blob/master/Makefile#L57
Looks like a bug; perhaps it should be weaveworks/weave. @tomwilkie

I'm happy to look at the weave status one if @rade doesn't want to (I looked at part of it already). It would be good to get it merged today.

I've rebased this against the new fdp branch.

Looks like a bug; perhaps it should be weaveworks/weave. @tomwilkie
https://github.com/tomwilkie

Yup, that could be considered a bug.  Making it weave/works should fix it.
On Mon, Oct 12, 2015 at 3:24 PM, Matthias Radestock <
<EMAIL_ADDRESS>wrote:

Perhaps it's because they're both under the weaveworks github org?
https://github.com/weaveworks/weave/blob/master/Makefile#L57
Looks like a bug; perhaps it should be weaveworks/weave. @tomwilkie
https://github.com/tomwilkie
—
Reply to this email directly or view it on GitHub
https://github.com/weaveworks/weave/pull/1438#issuecomment-147412410.

Only two things are preventing me from pressing merge:

I don't know if we're planning to include updated users docs in this PR
I don't know if weave status is getting merged into this branch before it gets merged into master

Thoughts @dpw @rade?

I've (hopefully) fixed the coverage reporting on master. Somebody please do a re-base.
I am happy to defer user docs and weave status to separate issues.

Somebody please do a re-base.

If I rebase this, then @awh will have to rebase #1513, and I'll have to rebase #1522.  If the only impact on the coverage report is to exclude some go-odp files, is a rebase going to tell us anything new?

is a rebase going to tell us anything new?

a) whether my fix actually worked
b) whether the decrease in coverage really is (nearly)accounted for by go-odp

is a rebase going to tell us anything new?

a) whether my fix actually worked
b) whether the decrease in coverage really is (nearly)accounted for by go-odp

You could have answered both questions directly by branching your fix off this branch.
Rebased.

Rebased.

Samewise.

Coverage decreased (-12.06%) to 61.737%

That hasn't gone well :smile:

There seem to be some quite big coverage holes, e.g.

{fastdp,overlap_switch,sleeve} InvalidateShortIDs
fastdp expireMACs,expireFlows,touchFlow

There seem to be some quite big coverage holes

That's not the problem - in fact, only overlay.go has reduced in coverage due to the addition of eight untested lines. The report is still counting all the lines of the sources files in odp, except now it is reporting zero test coverage for them all...

There seem to be some quite big coverage holes, e.g.

{fastdp,overlap_switch,sleeve} InvalidateShortIDs

Because none of the smoke tests have nearly enough peers to cause short ID collisions.

fastdp expireMACs,expireFlows,touchFlow

Because we don't run the smoke tests for long enough for flows to be expired (in the same way we never see MAC cache expiries).
At 74% and 78%, the coverage of the two big new files (fastdp.go and overlay_switch.go) is about average.

Can you think of an easy way to test InvalidateShortIDs?

Can you think of an easy way to test InvalidateShortIDs?

No, nothing that isn't a case of Volkswagening.

Can you think of an easy way to test InvalidateShortIDs?
No, nothing that isn't a case of Volkswagening.

And note that:

InvalidateShortIDs is just logging and locking around a call to deleteFlows, which is covered.

The unit tests verify that InvalidateShortIDs notifications occur when appropriate.

So it seems reasonable to check the code in InvalidateShortIDs by inspection.

coverage sorted.

@dpw @rade I don't think the current short ID discussion in #1522 has any bearing on this PR - any final comments before I press merge?

merge it


Norminette not found

Hi,
I've been working with VS Code on macOS Monterey (version 12.7.3) and the VS Code version I'm using is 1.87.2 (Universal). Recently, I've encountered an issue where I can't utilize the extension due to an error stating, "Norminette not found [link to the Norminette GitHub repo]". I've ensured that my Norminette version is up-to-date (3.3.55), installed with its default settings. Although I've attempted to update it, and it operates correctly when executing the "norminette" command directly, the error persists within VS Code. My Python and pip installations are also current, with versions 3.11.7 and 24.0, respectively, located at /usr/local/lib/python3.11/site-packages/pip (python 3.11).
I'm at a loss for what might be causing this issue. Do you have any suggestions?
Thank you for your assistance.

You could try to set codam-norminette-3.command to python3 -m norminette, or if that doesn't work try to set it to the actual path of norminette (You can get it by running where norminette in a terminal)

Thank you for your quick reply.
I changed this in my settings.json "codam-norminette-3.command": "python3 -m norminette", and it worked perfectly.
Thanks.