True dedicated CPU support with a side-car emulator container

This PR aims to bring support for VMs with true dedicated CPUs, designed in a simple, optimized, resilient and extensible way that fits to Kubevirt and Kubernetes' design and to future possible extensions or changes. Not only that this PR would bring better support for VMs with dedicated CPUs, it would open a door for further cgroup-related customizations and extensions in the future.
Context
Background
I had a talk regarding this topic at FOSDEM 2023. Please note that implementation had changed since I gave the talk, so information is not 100% accurate there. Saying that, it explains this effort and can be a good starting point to understand this PR and the reasoning behind it.

Lecture: https://www.youtube.com/watch?v=uoz2_9XnhAs
Slides PDF: https://fosdem.org/2023/schedule/event/vai_journey_supporting_vms/attachments/slides/5551/export/events/attachments/vai_journey_supporting_vms/slides/5551/Slides_PDF
Slides PPTX: https://fosdem.org/2023/schedule/event/vai_journey_supporting_vms/attachments/slides/5552/export/events/attachments/vai_journey_supporting_vms/slides/5552/Slides_PPTX

Up until not long ago, Kubevirt didn't fully support dedicated CPUs. In practice, although dedicated pCPUs
were reserved for the compute container, these pCPUs were shared between the guest CPUs (which are,
from the host perspective, vCPU threads) and our managerial processes like libvirtd and virtlogd, and QEMU
low-priority threads. In other words, when we told the guest it had X dedicated CPUs, we half-lied since
these pCPUs are being dedicated to both the guest and a group of other processes/threads the guest is
not aware of and shouldn't care about. As the CPUs aren't really dedicated in this case, this may seriously
affect realtime VMs or VMs that must have very small latency. Because of what's explained above,
there was a need to support true dedicated CPUs to the guest that are owned exclusively by it.
Housekeeping approach
The "housekeeping cgroup" PR is meant to solve the above problem: https://github.com/kubevirt/kubevirt/pull/8233.
I'll try to briefly explain this approach first.
Let's say that a VM asks for X dedicated CPUs. The approach that is followed in the PR can be briefly explained as:

A new virt-launcher pod is created. It would demand X+1 CPUs from Kubernetes.

That is, one more CPU is requested on top of what the user requested.

New child cgroup is created for the compute container. This is the "housekeeping" cgroup. This cgroup is allocated with 1 of the requested CPUs.

In v1: the new cgroup is creted only under the cpuset controller.
In v2: the new cgroup is threaded. see cgroup's v2 thread mode for more info.

virt-hanler would move all of the threads, except for the vCPU threads, into the housekeeping cgroup.
In the compute container we are now left with the X CPUs that the user requested. The vCPU threads would be pinned to these CPUs.

Problems with housekeeping approach
While this approach guarantees the guest to have true dedicated CPUs, and is definitely a step forward, it also introduces several problems:

We request one extra CPU that we don't actually need. Ideally, the "housekeeping" tasks would run on shared CPUs - not dedicated ones.
This approach is designed to deal with what we don't care about. That is, we want the vCPUs to behave in a certain way, so attach every other process/thread to a different cgroup. This should be reversed - we should configure only the vCPU threads and leave all of the other tasks as is.
I want to stress that this point is crucial. For example, in v2 the housekeeping cgroup is threaded, which means that it's now bound to some serious limitations like being able to use only a subset of cgroup subsystems (also known as "threaded controllers"). For example, this means that controllers like io, memory, hugetlband misc can't be used in the compute container anymore.
This approach is not resilient to new spawned processes in the compute container. As an example, if k exec ... is executed, a new process is spawned into the compute cgroup by default (which, as a reminder, now serves as a vCPU-only cgroup with dedicated CPUs). This breaks the dedicated CPU guarantee - the new process would share CPU time with the guest - which means that the CPUs aren't really dedicated, so this means this approach is broken by design. Ideally, any new process that's spawned to the compute container would be considered as a "regular"/"housekeeping" task.

This was originally discussed in this comment.
So, this approach hurts us in the following ways:

It is wasteful w.r.t. CPU resources.
It's hard to extend.
It seriously limits the way we can use cgroups in the compute container.
It's not intuitive and more complex.
It's not resilient to new spawned processes, therefore is broken by design (see point 3 above).

Dedicated cgroup approach (this PR)
Although aimed to solve the same problem, this approach is different from the housekeeping approach
presented above. The main idea is to create another side-car container to the virt-launcher Pod that would
be dedicated specifically to emulation, the "emulator" container. This container would run the QEMU process
and all of its threads.
Here is how it would look like:

Some notes and clarifications:

The "ambassador" process simply opens a socket and accepts connections. This is being done in order to find the process' PID, that would lead to finding the cgroup path. The same method is currently used to find the compute container's cgroup path.

In the future, we can use this process as a middle-man between libvirtd and QEMU so that the QEMU process would be spawned directly into this container.

The "emulator cgroup" doesn't run any processes/threads directly. All of the threads are spread between its children.
The "housekeeping" could also be extended in the future for finer-grained control over the different QEMU threads.

The container is carefully designed so that its cgroup hierarchy would be able to support true dedicated CPUs tothe guest, but would also provide other benefits in terms of:

Accountability: the structure provides finer-grained information and statistics regarding memory usage. IOW, we would be able to accurately know which memory is accounted for by threads / processes.
Fine-grained control: ability to separately control emulator / compute resources, priorities and other specifications. In addition, we could control each cgroup differently.
Extensibility: Leave the door open for more extensions and changes in the future. Specifically, remove the limitations that currently affect the compute container, as described above.
Stability: For example, if the compute container crashes for some reason, the emulator container (and the guest itself) would continue to run.
Security: With this design the guest is much more isolated since it resides in a different container.

So, to give a concrete example, let's say that once again a VM asks for X dedicated CPUs:

virt-launcher would be defined with an additional side-car container, the "emulator" container.

This container would be defined with X dedicated CPUs.
"compute" container stays as-is. That is, we don't ask for dedicated CPUs, but a shared cpu amount (e.g. 200m / 0.2).

virt-handler would initialize the cgroup hierarchy inside the emulator container
virt-handler would move the qemu container and its threads to the new container.

With this approach we avoid wasting an extra CPU, we configure only the vCPU threads which we
care about, leave the compute container as-is, and in general leave the door open for further refinements
that we may want to do in the future.
Important links
Original "housekeeping" approach PR: https://github.com/kubevirt/kubevirt/pull/8233
Dedicated cpus approach POC PR: https://github.com/kubevirt/kubevirt/pull/8653

This PR is implemented in a much more naive and straight-forward way.It is a good reference to understand the underlying implementation.Cgroups manual pages: https://man7.org/linux/man-pages/man7/cgroups.7.html

Release note:

Brings support for VMs with true dedicated CPUs, designed in a simple, optimized, resilient and extensible way that fits to Kubevirt and Kubernetes' design and to future possible extensions or changes.

/test pull-kubevirt-e2e-k8s-1.24-sig-compute
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.24-sig-storage
/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2

/test pull-kubevirt-e2e-k8s-1.24-sig-compute
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.24-sig-storage
/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2

/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2

/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.24-sig-storage

/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.24-sig-storage

/test pull-kubevirt-e2e-k8s-1.24-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-storage-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.25-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.25-sig-storage-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.26-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.24-sig-storage
/test pull-kubevirt-e2e-k8s-1.25-sig-compute
/test pull-kubevirt-e2e-k8s-1.25-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.25-sig-storage
/test pull-kubevirt-e2e-k8s-1.26-sig-compute

/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2

After a lot of work, this PR is now ready to review :)
@vladikr and @vasiliy-ul, I'd appreciate your review in particular.
Some questions / notes / open problems:

Currently, in https://github.com/kubevirt/kubevirt/pull/8869/commits/04027be22a21bd4d4fb7ffe0f90a92be67f6b7e7, I'm using a workaround to identify the dedicated-cpu container's cgroup path. Basically, I'm calculating a very large deterministic and unique value X to a VMI. The dedicated-cpu container executes a while <forever>: sleep X command, then virt-handler can track the process (by ps -o pid,comm,args -A | grep X) and hence its cgroup path. One alternative to this workaround is to create another EmptyDir volume for the dedicated-cpu container and create a file with the right cgroup path / PID in it. Does that sound reasonable? In addition, should I use a fedora or any other image as base, or would it be better to create a dedicated binary for that? WDYT
I've had some issues testing v2 locally. The new functional tests try to run commands on the node by running them on the relevant virt-handler Pod, but errors like Permission denied started to pop up. Interestingly, when trying to manually execute these commands on virt-handler (i.e. k exec -it virt-handler ..., I've wintnessed no such errors. In addition, some v2 lanes had already passed on this PR. Let's see what CI says and try to figure out how we can solve it.
In https://github.com/kubevirt/kubevirt/pull/8869/commits/dc53686c591798ea1cef304c1992f5aeca032d49#diff-d33f64d6aa2a0e559b8c4088c3b889db17dc314bb0aefffb16844c56ad46ffdeR112, I'm relying on the fact that Kubevirt is being tested with CRIO. Is this acceptable?
Not that it should block this PR, but for the record, there's a problem with cgroup unit tests since they refer to the cgroup version of the environment that runs them. We should make better mocks so that all unit tests are running on both versions of cgroups.

Thanks in advance!

/cc @vasiliy-ul @matosatti

/test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2

/retest-required

So seems like we also have a problem with some storage tests.
The error comes from virt-handler that can't seem to apply the device rules:
err: exit status 1: falling back to securejoin: openat2 /sys/fs/cgroup: no such file or directory
Error: cannot set cgroup resources.
err: open devices/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod014aaf1c_5595_45ed_aa49_d912f67f9638.slice/crio-0a01c0202ba6e5a3df6111177e8cec8787188edd0b437499b13133397f52eef9.scope/devices.list: no such file or directory

But when trying to manually run it on virt-handler the path does seem to exist:
> K exec -it virt-handler-76hp2 -- cat /proc/1/root/sys/fs/cgroup/devices/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod014aaf1c_5595_45ed_aa49_d912f67f9638.slice/crio-0a01c0202ba6e5a3df6111177e8cec8787188edd0b437499b13133397f52eef9.scope/devices.list
selecting docker as container runtime
Defaulted container "virt-handler" out of: virt-handler, virt-launcher (init)
c 136:* rwm
c 10:238 rw
c 10:232 rw
c 10:200 rwm
c 5:2 rwm
c 5:0 rwm
c 1:9 rwm
c 1:8 rwm
c 1:7 rwm
c 1:5 rwm
c 1:3 rwm
b *:* m
c *:* m

I've disabled SELinux on my CI nodes but it didn't help.
Will try to investigate.

/cc @jean-edouard

notes about cgroups v2
cgroup v2 introduces new limitations, such as the "internal process rule" and the fact that in
threaded mode all of the threads must reside in their parent process' subtree. This means we >can't move only the vCPU threads to the dedicated-cpus cgroup without moving the whole process >(which is qemu-kvm). This is a problem since some of the threads of this process are low-priority >threads.

Then this solution is suboptimal: you are creating two different versions of cgroup arrangements, one for cgroups v1 and another for cgroups v2. This is confusing.
I suggested to look at the fact that:
"A process can be moved into a threaded subtree by writing its PID
to the cgroup.procs file in one of the cgroups inside the tree.
This has the effect of making all of the threads in the process
members of the corresponding cgroup and makes the process a
member of the threaded subtree.  The threads of the process can
then be spread across the threaded subtree by writing their
thread IDs (see gettid(2)) to the
cgroup.threads files in different cgroups inside the subtree.
The threads of a process must all reside in the same threaded subtree."
This seem to indicate that the following arrangement is possible:
qemu-kvm parent  cgroup               cpumask = isolated cpu list + shared cpu
cgroup housekeeping  (cpumask=shared cpu)         cgroup dedicated cpus   (cpumask=isolated cpu list)
qemu-kvm main thread                                              qemu-kvm vcpus
So the cgroup structure would be:
qemu-kvm parent cgroup -> {cgroup housekeeping, cgroup dedicated cpus}
Did you look into that?

This PR fallbacks to allocating an extra CPU for the v2 case. This way, the low-priority threads of > qemu-kvm can run under another CPU, similarily to the housekeeping approach. Currently, I can't > think of a better solution to get around that. I would say that currently the vast majority of the >workloads still run on cgroup v1 (Kubernetes supports v2 in a stable way only from v1.25), so we >have some time to figure out a solution for this case. Worst case scenario, we stay with the current > extra CPU problem only for v2 cases, but still improve the overall design.

One has to use cgroups v2 in threaded mode.

For example, this means that controllers like io, memory, hugetlb and misc can't be used in the
compute container anymore.

They can't be used in the threaded subtree, that is all. But they can be used in parent cgroup (so this is not an actual argument).

We request one extra CPU that we don't actually need. Ideally, the "housekeeping" tasks would >run on shared CPUs - not dedicated ones.

This should be selectable, sers might want housekeeping tasks to run on shared CPUs (but its a nice improvement, and removes a current limitation).

Then this solution is suboptimal: you are creating two different versions of cgroup arrangements, one for cgroups v1 and another for cgroups v2. This is confusing.

Well, since cgroups v2 completely breaks backward compatibility and is designed completely differently this kind of makes sense to me. The whole design of our cgroups manager, for example, is that it has two different implementations for v1 and v2. That's how runc's cgroup manager is also designed.

cgroup housekeeping (cpumask=shared cpu) cgroup dedicated cpus (cpumask=isolated cpu list)
qemu-kvm main thread qemu-kvm vcpus
So the cgroup structure would be:
qemu-kvm parent cgroup -> {cgroup housekeeping, cgroup dedicated cpus}
Did you look into that?

Not entirely sure I understood you correctly, but one of the problems is that we can't have both shared CPUs and dedicated CPUs under the same cgroup. That's a limitation from Kubernetes, as dedicated cpus is being applied per-container. IOW, either a container (and its cgroup) have only shared CPUs or only dedicated CPUs.
So what you suggested makes sense from a pure cgroups perspective, but I don't see how it could be implemented on top of Kubernetes.

This PR fallbacks to allocating an extra CPU for the v2 case. This way, the low-priority threads of > qemu-kvm can run under another CPU, similarily to the housekeeping approach. Currently, I can't > think of a better solution to get around that. I would say that currently the vast majority of the >workloads still run on cgroup v1 (Kubernetes supports v2 in a stable way only from v1.25), so we >have some time to figure out a solution for this case. Worst case scenario, we stay with the current > extra CPU problem only for v2 cases, but still improve the overall design.

One has to use cgroups v2 in threaded mode.

Threaded mode is a concept that exists only in v2.
Not entirely sure I understand your point here.

For example, this means that controllers like io, memory, hugetlb and misc can't be used in the
compute container anymore.

They can't be used in the threaded subtree, that is all. But they can be used in parent cgroup (so this is not an actual argument).

In the current implementation everything except for the vcpus resides inside a threaded subtree, so effectively no controllers can be used for any process/thread other than the vcpus. That's a problem as it prevents us from using more controllers on non-vcpu threads in the future.
That's actually one of the main side-effects and concerns I have with the current approach. We deal with anything else other than vcpus in order to pin cpus to the vcpu threads.

We request one extra CPU that we don't actually need. Ideally, the "housekeeping" tasks would >run on shared CPUs - not dedicated ones.

This should be selectable, sers might want housekeeping tasks to run on shared CPUs (but its a nice improvement, and removes a current limitation).

While it's already possible to make io-threads run on a dedicated CPU, I don't see a real reason why something like virtlogd for example should run on a dedicated CPU. And in general it's wrong to assume that every "housekeeping" process has the same priority. For example, in the future we can decide to add a very low priority process to the compute container - as a side-effect it would also run on a dedicated CPU.
As said above - my main concern with the current design is that instead of configuring only the things we really care about (vcpus in this case) we configure anything else in a way that will be hard to extend in the future.

/test pull-kubevirt-e2e-k8s-1.25-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.24-sig-storage
/test pull-kubevirt-e2e-k8s-1.25-sig-compute
/test pull-kubevirt-e2e-k8s-1.25-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.25-sig-storage

/test pull-kubevirt-e2e-k8s-1.25-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
/test pull-kubevirt-e2e-k8s-1.24-sig-compute
/test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.24-sig-storage
/test pull-kubevirt-e2e-k8s-1.25-sig-compute
/test pull-kubevirt-e2e-k8s-1.25-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.25-sig-storage

I'm happy to say this PR is finally ready to review.
Can you please take another look? @vladikr @vasiliy-ul @matosatti
Please bear in mind that the implementation had changed in many ways.
Please read the updated description before getting to the code itself.
Thanks a lot!

Itamar,
Looking good, thanks for doing this!
Some general comments:

Can you please add a test for "k exec" to double-check it won't
affect isolated CPUs? (and to avoid future regressions).

Would be nice to make sure that the logic where vcpus are stopped by the time threads
are placed in the proper cgroups (i think this is true, but
if we fail to do this nasty and hard to debug bugs will show
up).

BTW, perhaps making this behaviour (to create the VM stopped,
and then later once its fully initialized then start the VM)
default for all cases might be beneficial (since then the code
has to deal with a single unified behaviour, which results in
greater testing coverage, etc).

Can you please show live cgroup hierarchy in the filesystem
(showing "tasks" and "cpuset.cpus" files for each cgroup, with
and without IsolateEmulatorThread)? Just as a comment in the thread.

Great work!
So trying to summarize the advantage over the current state,this seems to be the key difference:

"compute" container stays as-is. That is, we don't ask for dedicated CPUs, but a shared cpu amount (e.g. 200m / 0.2).

Basically moving the emulator side-car to the Guaranteed QoS cgroup. That means we will reserve 1/5th compared to before. And this 1/5th is still counted on the scheduler, but at least not a full CPU, right?
This is definitely an improvement.
Still I wonder:
Did you also consider letting virt-handler maintain a common cgroup hierarchy where we put these processes in, without additional side-cars?
I think that would give us the chance to have QoS guaranteed guests where we don't need to add side-cars in the same QoS class, as well as the flexibility to "overcommit" the way we see fit.
It would also not be less-secure considering misbehaving virt-launchers, because also right now we just place the processes in the guaranteed cgroup and give them worst case the time they requested. We can keep a similar process in our onw cgroup hierarchy.
That would open up the possibility to move all kinds of CPU and memory overhead to common cgroups (we can still have subroups for different purposes if we like), where we can prioritize less-performance-critical overhead much lower and stronger overcommit.
That may tackle the core problem we are facing better from a long-term perspective. What do you think?

Hey @rmohr, thanks for your feedback!

Basically moving the emulator side-car to the Guaranteed QoS cgroup. That means we will reserve 1/5th compared to before. And this 1/5th is still counted on the scheduler, but at least not a full CPU, right?

When allocating dedicated CPUs to a pod, all of the containers must be of guaranteed QoS, but some of them could be allocated with shared CPUs. The idea is that the compute container would be allocated with shared cpus (although being guaranteed QoS) and the "emulator" side-car container would be allocated with dedicated CPUs. While 0.2 is just an example, the key idea is that we wouldn't have to run our virt infra (especially the low-priority threads) on dedicated CPUs and we would be able to allocate resources to it separately. Obviously, all allocated resources would be accounted for the scheduler.

Still I wonder:
Did you also consider letting virt-handler maintain a common cgroup hierarchy where we put these processes in, without additional side-cars?

Yes indeed.
This was the original thought. However, the problem is that in Kubernetes' memory allocation model it is impossible to allocate both dedicated and shared CPUs to the same container. Ideally, we would allocate X dedicated CPUs and Y shared CPUs to the compute container and create a common cgroup hierarchy in it, but as said, this is impossible.
The idea of a side-car container came from this fact. It basically enforces Kubernetes into allocating two cgroup hierarchies - one with dedicated and the other with shared CPUs. Once we have that, we can use these cpu sets to allocate dedicated CPUs only to vcpu threads.
But, further to the explained above, TBH I think that this approach has other advantages. The emulator (QEMU) process itself and the other virt infra in the compute container have very different priorities and it makes sense to me that they would be configured and isolated differently:

We could prioritize the containers differently
Different permissions and capabilities
Different resource allocations
Better guest isolation
Stability: guest keeps running even if compute container restarts

So IMHO the technical difficulty actually pushes us to a better overall design that would open the door for future fine-tuning that would be done separately to the high-priority QEMU process and our low-priority virt infra. I think that in the future we should consider adopting this approach even for non-dedicated-cpus VMs.
What do you think?
Thanks again for this discussion

When allocating dedicated CPUs to a pod, all of the containers must be of guaranteed QoS, but some of them could be allocated with shared CPUs. The idea is that the compute container would be allocated with shared cpus (although being guaranteed QoS) and the "emulator" side-car container would be allocated with dedicated CPUs. While 0.2 is just an example, the key idea is that we wouldn't have to run our virt infra (especially the low-priority threads) on dedicated CPUs and we would be able to allocate resources to it separately. Obviously, all allocated resources would be accounted for the scheduler.

Thanks for confirming.

Still I wonder:
Did you also consider letting virt-handler maintain a common cgroup hierarchy where we put these processes in, without additional side-cars?

Yes indeed. This was the original thought. However, the problem is that in Kubernetes' memory allocation model it is impossible to allocate both dedicated and shared CPUs to the same container. Ideally, we would allocate X dedicated CPUs and Y shared CPUs to the compute container and create a common cgroup hierarchy in it, but as said, this is impossible.

I think we may talk across each other. I agree that the current housekeeping sub-cgroup approach can't overcome mixing things. What I meant is a common cgroup hierarchy for all compute containers overhead. virt-handler would create and maintain its own top-level cgroup where we would move the processes in, freely prioritize and so forth.
For example: If I create 300 VMs on the machine, with your approach we are definitely in a better spot from resource consumption, given that we do not request a full CPU. Still it is a lot. In your example we would still allocate at least 20 CPUs just to the overhead from the scheduler perspective.
If virt-handler would have its own cgroup, it could potentially work within the scope of system-daemon reserved cgroups, or in its completely reserved set of cgroups, where e.g. 3 CPUs for everything may be sufficient.

For example: If I create 300 VMs on the machine, with your approach we are definitely in a better spot from resource consumption, given that we do not request a full CPU. Still it is a lot. In your example we would still allocate at least 60 CPUs just to the overhead from the scheduler perspective.

And following up on these numbers here. Even with a number of 60 CPUs for overhead (defnitely better than 300 reserved CPUs), it is already visible that this practically means that even with this improvement, we take away a huge amount of schedulabe resources (also on memory btw.) which gives us a hard time being competitive with non-k8s based virtualization solutions. I therefore wonder if it is worth going down this side-car approach, or trying to solve this more aggressively.

When allocating dedicated CPUs to a pod, all of the containers must be of guaranteed QoS, but some of them could be allocated with shared CPUs. The idea is that the compute container would be allocated with shared cpus (although being guaranteed QoS) and the "emulator" side-car container would be allocated with dedicated CPUs. While 0.2 is just an example, the key idea is that we wouldn't have to run our virt infra (especially the low-priority threads) on dedicated CPUs and we would be able to allocate resources to it separately. Obviously, all allocated resources would be accounted for the scheduler.

Thanks for confirming.

Still I wonder:
Did you also consider letting virt-handler maintain a common cgroup hierarchy where we put these processes in, without additional side-cars?

Yes indeed. This was the original thought. However, the problem is that in Kubernetes' memory allocation model it is impossible to allocate both dedicated and shared CPUs to the same container. Ideally, we would allocate X dedicated CPUs and Y shared CPUs to the compute container and create a common cgroup hierarchy in it, but as said, this is impossible.

I think we may talk across each other. I agree that the current housekeeping sub-cgroup approach can't overcome mixing things. What I meant is a common cgroup hierarchy for all compute containers overhead. virt-handler would create and maintain its own top-level cgroup where we would move the processes in, freely prioritize and so forth.
For example: If I create 300 VMs on the machine, with your approach we are definitely in a better spot from resource consumption, given that we do not request a full CPU. Still it is a lot. In your example we would still allocate at least 60 CPUs just to the overhead from the scheduler perspective.
If virt-handler would create and maintain its own cgroup, it could potentially work within the scope of system-daemon reserved cgroups, or in its completely reserved set of cgroups, where e.g. 3 CPUs for everything may be sufficient.

Thanks for elaborating.
So IIUC, your suggestion is for virt-handler to create a sub-cgroup for virt infra. Then we would allocate the compute container with a dedicated cpuset, then move the virt-infra threads to the new virt-handler sub-cgroup?
That's an interesting idea indeed. Thanks for sharing that.
One challenge I see in this approach is that even when we're talking about the QEMU process itself, some of its threads should run on shared CPUs (e.g. vnc-worker, and basically everything other than the vCPU threads). With cgroups v2, with the concept of "threaded cgroups", all threads must reside in their parent process' subtree. So moving only some threads to the handler's hierarchy is a problem.

For example: If I create 300 VMs on the machine, with your approach we are definitely in a better spot from resource consumption, given that we do not request a full CPU. Still it is a lot. In your example we would still allocate at least 60 CPUs just to the overhead from the scheduler perspective.

And following up on these numbers here. Even with a number of 60 CPUs for overhead (defnitely better than 300 reserved CPUs), it is already visible that this practically means that even with this improvement, we take away a huge amount of schedulabe resources (also on memory btw.) which gives us a hard time being competitive with non-k8s based virtualization solutions. I therefore wonder if it is worth going down this side-car approach, or trying to solve this more aggressively.

I definitely agree.
Actually, I've sent a mail to virt-devel regarding a small "libvirt shim" suggestion.
The idea is that our containers would start with this shim (basically a unix socket) that would communicate with libvirtd. This way libvirtd would be able to communicate and control guests that are running inside of containers. Ideally, we would end up with a single libvirtd instance that would control the different guests from the host side.
This is, however, just a suggestion. But I think that trying to move in this direction would be ideal.

which gives us a hard time being competitive with non-k8s based virtualization solutions.

And another small note:
The problem you're outlining here is an important one, but essentially it was like that from the dawn of Kubevirt IIUC. While I fully agree we should address that problem, I think this PR does not make things worse.

I definitely agree. Actually, I've sent a mail to virt-devel regarding a small "libvirt shim" suggestion. The idea is that our containers would start with this shim (basically a unix socket) that would communicate with libvirtd. This way libvirtd would be able to communicate and control guests that are running inside of containers. Ideally, we would end up with a single libvirtd instance that would control the different guests from the host side.

Ah, here I am not sure this is a preferable model. Let's consider solutions without involving minor resource-optimizations without looking at a more general solution pattern first.

One challenge I see in this approach is that even when we're talking about the QEMU process itself, some of its threads should run on shared CPUs (e.g. vnc-worker, and basically everything other than the vCPU threads). With cgroups v2, with the concept of "threaded cgroups", all threads must reside in their parent process' subtree. So moving only some threads to the handler's hierarchy is a problem.

Yes there are definitely interesting challenges. Just to highlight one out of many hints hint: In 2020 some qemu-multi-process work landed in qemu. Since it got merged in mainline  qemu in 2020, I had a hard time finding information on its progress and limitations after it got accepted, but it sounds like a chance for us as well.

And another small note:
The problem you're outlining here is an important one, but essentially it was like that from the dawn of Kubevirt IIUC. While I fully agree we should address that problem, I think this PR does not make things worse.

Yes, I just want to point out that I think we are not focusing enough on getting an idea of a long-term perspective and just dragging us from one minor fix to the next minor fix. It is probably ok for me to get this in, but even such a change has other user-facing drawbacks, like in this case adding another variable with the sidecar which can slow down the VM startup time (and a sidecar we have to wait for can significantly impact this, something which I for instance have to care in my current position as well and have to see how to live with this change) and it gets harder the more of these fixes we accumulate to not significantly impact other areas.
Maybe we can form a kind of a taskforce on looking into that from a broader perspective.

Yes there are definitely interesting challenges. Just to highlight one out of many hints hint: In 2020 some qemu-multi-process work landed in qemu. Since it got merged in mainline qemu in 2020, I had a hard time finding information on its progress and limitations after it got accepted, but it sounds like a chance for us as well.

Looks like this effort shifted to vfio-user which would also bring a process model which we may benefit from.

Looks like this effort shifted to vfio-user which would also bring a process model which we may benefit from.

@rmohr where does this effort continue?

Looks like this effort shifted to vfio-user which would also bring a process model which we may benefit from.

@rmohr where does this effort continue?

The code is in branches in https://github.com/oracle/qemu. There was an email on the qemu mailing list (where someone wanted to use the multi process work and got explained that it shifted to that model. I can't find the exact post right now though. Should have linked it right away ...

/remove-lifecycle stale


fix issues with alert component

Question
Answer

Issue or Enhancement
Enhancement

License
MIT

What's in this PR?
This PR fixes #859
Changes in alert.blade.php ensure that:

If title is not defined, the h5 header with icon and title is not generated
Passing title="" generates the header with icon and empty title.
Passing icon="" does not generate the font-awesome icon element.

Change in Alert.php ensures that if a custom theme attribute is passed, it does not generate an error.
Checklist

[x] I tested these changes.
[x] I have linked the related issues.

@adwiv Thanks, I will give this a review later...

I have improve the PR to:
1- Remove default theme, theme is now optional. When no theme, a border will be rendered on the alert.
2- Title and icon are now optional. You can define an alert header with only icon, or only title, or without header if no icon and no title is provided.
{{-- Minimal --}}

    <x-adminlte-alert>Minimal example</x-adminlte-alert>

{{-- Minimal without icon --}}

    <x-adminlte-alert title="Well done!" dismissable>
         Minimal example
    </x-adminlte-alert>

@Shidersz This looks good. Only one minor issue - iconless theme with text like this bootstrap example is not possible using
<x-adminlte-alert theme="success" title="Well done!" dismissable>
         Minimal example
    </x-adminlte-alert>

We can still use the class attribute, but that generates an additional border class which overrides the border in alert-success.
    <x-adminlte-alert class="alert-success" title="Well done!" dismissable>
        Minimal example
    </x-adminlte-alert>

Adding a border class only if no theme and no class attribute was specified can fix this.

On further experimentation, the problem with border I reported in last comment can be fixed by specifying additional class attribute.
<x-adminlte-alert class="alert-success border-success" title="Well done!" dismissable>
        Minimal example
</x-adminlte-alert>

So I guess we are good to go. We will however need to update the documentation for this usecase.

@adwiv ok, thanks for your time, I will update they documentation later. I'm currently away from computer.

@adwiv I have made a minor change, so now you can use icon="" to remove default icon from a theme:
<x-adminlte-alert theme="success" title="Well done!" icon="" dismissable>
    Minimal example
</x-adminlte-alert>

@Shidersz Looks great. I think all the concerns are addressed now.


Setting the same cache entry twice during the same second makes the 2nd set fail.

When setting the exact same cache entry during the same second, the 2nd set() returns false, making it seem like the set has failed, but it didn't.
The yii\caching\DbCache his setValue() function first tries an UPDATE to update the cache, and if that fails, it tries an INSERT via the addValue() function. This seems fine, except that the UPDATE query returns the number of rows changed, and by default MySQL returns 0 changed rows when the UPDATE is identical to the existing value. I'm not sure what other databases do.
This causes setValue() to think the UPDATE has failed and tries a addValue() call, what throws an DUPLICATE KEY Exception since the data is already there. This is handled with a return false causing the calling functions to think the cache set has failed.
A fix is to set the PDO::MYSQL_ATTR_FOUND_ROWS attribute to true. This causes MySQL to return the number of found rows instead the number of changed rows in a UPDATE call.
I'm not sure if this is the correct behavior, so a better way could be to change setValue() to see the difference between an existing identical cache entry, or a non-existing cache entry.
Workaround
return [
    ...
    'components' => [
        ...
        'db' => [
            'class' => 'yii\db\Connection',
            'dsn' => 'mysql:host=*hostname*;dbname=*database*',
            'username' => '*username*',
            'password' => '*password*',
            'attributes' => [
                PDO::MYSQL_ATTR_FOUND_ROWS => true,
            ],
        ],
    ],
];

What steps will reproduce the problem?
if (!Yii::$app->cache->set('key', 'value', 86400))) {
  throw new Exception("O my god, the cache set has failed. (1)");
}

if (!Yii::$app->cache->set('key', 'value', 86400))) {
  throw new Exception("O my god, the cache set has failed. (2)");
}

Returns:
O my god, the cache set has failed. (2)
if (!Yii::$app->cache->set('key', 'value', 86400))) {
  throw new Exception("O my god, the cache set has failed. (1)");
}

sleep(1); // sleep a second, the expiry value will change

if (!Yii::$app->cache->set('key', 'value', 86400))) {
  throw new Exception("O my god, the cache set has failed. (2)");
}

Returns nothing.
Additional info

Q
A

Yii version
2.0.12

PHP version
7.0

Operating system
Ubuntu 16.04

The only way I can think of now is do to a COUNT() query so you know if you need to UPDATE or INSERT. Is that a good approach to fix this bug?

Is it reproducible in a unit test (it should be)?

Issuing count() doesn't sound right to me. It hurts both performance (cache is all about it, right) and atomicity.

I haven't looked at the unit tests.
The only way to fix it without impacting performance (so no COUNT(*) query) seems to be to set PDO::MYSQL_ATTR_FOUND_ROWS => true by default then, but all databases use the same yii\db\Connection class, so that doesn't seem like it is the right place. Also, it might not be the behaviour what is expected. It seems okay to expect that update() returns the number of changed rows, it just seems wrong to expect that the number of changed rows means the update query was succesful.

@samdark I've tried running the tests locally, and it seems they are already failing.
$ phpunit -v tests/framework/caching/DbCacheTest.php
Applying compatibility patch for PHPUnit 6...
PHPUnit 4.8.34 by Sebastian Bergmann and contributors.

Runtime:	PHP 7.0.14
Configuration:	/project/phpunit.xml.dist

..F...............

Time: 9.68 seconds, Memory: 6.00MB

There was 1 failure:

1) yiiunit\framework\caching\DbCacheTest::testSet
Failed asserting that false is true.

/project/tests/framework/caching/CacheTestCase.php:89
/project/vendor/phpunit/phpunit/phpunit:52

FAILURES!
Tests: 18, Assertions: 52, Failures: 1.

The CacheTestCase on line 89, in testSet(), does this:
        $this->assertTrue($cache->set('string_test', 'string_test'));

I guess this fails because on line 77, in  the prepare(), it does this:
        $cache->set('string_test', 'string_test');

I assume if this happens in the same second, the test on line 89 fails.
Are you sure that travis is running also database tests?

It does. These aren't excluded.

This test makes sure that the records are added on the same second (since the test already allows mocking the time):
This will fail:
    public function testUpdateIdenticalRecord()
    {
        $cache = $this->getCacheInstance();

        static::$time = \time();
        $this->assertTrue($cache->set('update_record', 'update_record', 60));
        $this->assertTrue($cache->set('update_record', 'update_record', 60));
    }

When doing a static::$time = \time()+1; after the first assertTrue, it works fine.


[FLINK-24597][state] fix the problem that RocksStateKeysAndNamespaceIterator would return duplicate data

…terator would return duplicate data

What is the purpose of the change
fix the problem that RocksStateKeysAndNamespaceIterator would return duplicate data
Brief change log

Added the logic of filtering the same keyAndNamespace to the next method of RocksStateKeysAndNamespaceIterator
add unit test for the case that with MapState and update userKey multiple times under the same key

Verifying this change
Added test that validates that duplicate data could be filtered in testIteratorUpdateMapStateMultipleTimes
Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): ( no /)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: ( no )
The S3 file system connector: ( no )

Documentation

Does this pull request introduce a new feature?  no)

@flinkbot run azure

@flinkbot run azure


Add better support for Razor syntax highlighting

Highlight js currently has aspx-csharp and cshtml.
I'm not sure what the difference is between the two but neither highlight Razor syntax very well.
This is what I get in Visual Studio:

This is what I get in the docs:

@vicancy Is this a DocFX issue, or is it an implementation issue on the repo side of things?

The aspx-csharp option sounds like it supports Web Forms syntax highlighting. cshtml is definitely what we want to use.

@scottaddie yes, we're supporting Web Forms right now. Adding cshtml highlighting is in our backlog.


Does this work with Python 3.5?

OS X + Python 3.5:
When invoking fab setup:
ImportError: cannot import name 'isMappingType'
It looks like Fabric isn't compatible with Python 3.5.

Fabric isn't supported in Python 3. The same author has released invoke, which is a nice alternative.

fabric3 works with Python 3.5.


Override value of count aggregator

Hi all,
We're using a count aggregator in most of our data sources, once in a while we get a request to migrate a set of events from some external system. We get these events aggregated, so that a single row represents multiple events.
In order to get the correct count metric, I was thinking about one of -

Generate duplicate input rows according to the aggregated count - naturally this would be a huge overhead for large number of events
Avoid using count aggregator in general - always use longSum instead, and make sure to send "count": 1 in all our regular/non-migrated events
I think it would be great (maybe there already is such a feature? :) ) to have the ability to configure fieldName on the count aggregator - so that -
a. If fieldName exists in the event JSON, its value will be used
b. Otherwise, the value will be assumed to be 1 (as count normally behaves)

Thanks!
Eran

Hi @erankor, the usual way to do this is (2): using longSum and adding a "count": 1 to the regular events.

Thanks @gianm

@gianm, a follow up question - if I change my schema now from count to longSum, so that new segments will be indexed with longSum while old segments were indexed with count, is it expected to cause any issue? (e.g. will a query that spans across the two types return the correct result)
My guess is that count gets translated internally to longSum, and if that is the case, a segment created with count should be equivalent to one created with with longSum. But just wanted to make sure...
Thanks!
Eran

@dragonls, it's the other way around, the old segments use count aggregation, so it doesn't matter that the events don't have the count field (it will only be problematic if I will reindex them with the new schema).
For new segments, I will make sure to add the count field to the JSONs before I update the schema to longSum.
The question was whether segments that were ingested with longSum are equivalent to segments that were ingested with count (my guess is that they are, but would be great to get a confirmation on this)

@erankor The results are not the same.
The result using count may be smaller than that using longSum. The records with the same dimensions in one segment will be count just as 1.


fix #1389

fix #1389

Codecov Report

Merging #1390 (adee4ac) into master (549adf6) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1390      +/-   ##
==========================================
- Coverage   70.67%   70.62%   -0.05%
==========================================
  Files         378      378
  Lines       32482    32482
==========================================
- Hits        22958    22942      -16
- Misses       7906     7917      +11
- Partials     1618     1623       +5

Impacted Files
Coverage Δ

encoding/gjson/gjson_api_encoding.go
65.95% <0.00%> (ø)

os/gspath/gspath_cache.go
82.60% <0.00%> (-10.87%)
:arrow_down:

net/gtcp/gtcp_pool.go
36.84% <0.00%> (-3.95%)
:arrow_down:

container/gtree/gtree_redblacktree.go
87.04% <0.00%> (-1.06%)
:arrow_down:

container/gtree/gtree_btree.go
89.30% <0.00%> (-0.40%)
:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 549adf6...adee4ac. Read the comment docs.


Implement ScalarUDF in terms of ScalarUDFImpl trait

Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/8712
Rationale for this change
ScalarUDF is a struct of function pointers for historical reasons. After https://github.com/apache/arrow-datafusion/pull/8578, ScalarUDFImpl is available, we can clean up its internal implementation to be in terms of that trait directly, simplifying the implementation greatly.
What changes are included in this PR?

Rewrite ScalarUDF to have a single Arc<dyn ScalarUDFImpl field
Add +Debug to ScalarUDFImpl which forces implementations to also derive Debug which I think is much better than the opaque FUNC that was there previously

Are these changes tested?
yes
Are there any user-facing changes?
New requirement to derive Debug for ScalarUDFImpl but I think that is good hygene anyways

Thank you for the review @crepererum


Add a timeline and replace the timestamp

Instead of having the timestamp just displayed as text, this shows it as a timeline aligned with the sparklines. It helps give a visual picture of where you are in the animation + adds a bit more context for the sparklines. Removed the precip raster when I was building this because GDP wasn't working and built using the updates in #219. Please ignore the styling in the outro frames - I didn't commit changes there.

In response to your pipeline brittleness comments with task tables - I am treating that as a completely separate effort and would like to focus this PR review on just the timeline and pipeline pieces that are needed for the timeline.

Final version after review & reconciliation:


Use a different color for folder icons in file dialogs

This makes them easier to distinguish from files for quick visual grepping.
This can also be used in projects by setting the FileDialog "folder" color. The default value (Color(1, 1, 1)) has no visual impact, for compatibility with existing projects.
Preview
Dark theme

Light theme

Thanks!


Xcode 9: Automatically detect selected provisioning profiles of the Xcode project

This way fastlane will just keep working with Xcode 9 even for setups that don't use match (yet).
Xcode 9 requires a mapping of bundle identifier and what provisioning profile to use. We use xcodeproj to find those values, but in a way that gym doesn't break if we can't detect those things
I also verified that things work fine if you pass non-existent bundle identifier/profiles, Xcode will just ignore them 👍
Fixes https://github.com/fastlane/fastlane/issues/9380 even more
Output when running it:

Error message if it's not auto-detected, catching Xcode error, and linking to the new docs :)

          1 Error

:no_entry_sign:
Tests have failed, see below for more information.

Tests:

Name
File

Gym Gym::PackageCommandGeneratorXcode7 generates a valid plist file we need
package_command_generator_xcode7_spec.rb

Gym Gym::PackageCommandGeneratorXcode7 reads export options from hash
package_command_generator_xcode7_spec.rb

Gym Gym::PackageCommandGeneratorXcode7 doesn't store bitcode/symbols information for non app-store builds
package_command_generator_xcode7_spec.rb

  Generated by :no_entry_sign: Danger

Does this PR take care of this change too? https://github.com/fastlane/fastlane/pull/9449

Nice catch @ohayon, seems like it's the same thing, but with a different approach. Let's make sure we use the better one 👍

As I mentioned in #9449, I like combining both approaches as they do not seem contradictive of each other.
I do want to report a small bug I encountered while testing this PR combined with mine:
When using workspace instead of project in gym options, gym fails to find the project file to infer from. The error I get is:
The plist file at path `/Users/*****/*****/*****.xcworkspace/project.pbxproj` doesn't exist.

@KrauseFx when do you believe I can expect this to be merged in?

Whops, I thought this was already merged, thanks for pinging me. Looking into this now 👍

Thanks for letting me know re workspace, I can reproduce @igor-makarov 👍

@igor-makarov Would you mind giving it another shot for your workspace?

Still not working on my config... Two issues:

Iterating on the workspace can produce groups other than a project (in my case, a GPX folder). My suggestion is to add current_match.end_with?(".xcodeproj") to the find_all block.
Not related to workspaces, but I have several base .xcconfig files that the project build settings depend on. The provisioning profile is specified there, like so:

PROVISIONING_PROFILE_SPECIFIER = ${PRODUCT_BUNDLE_IDENTIFIER} AppStore

Because of that, in my setup, current["PRODUCT_BUNDLE_IDENTIFIER"] returns nil.
However, attempting to use Xcodeproj method target.resolved_build_setting function doesn't seem to work as it returns the value resolved from the xcconfig, but doesn't resolve the fact the value references another property, in this case "$(XXXXXX_BASE_BUNDLE_PREFIX).Widget".
This seems to be a limitation of Xcodeproj and I don't know how easy it is to address it. Perhaps @dantoml can clarify what can be done about this?

@KrauseFx any updates on getting this merged?

Yeah, it would be awesome if you could provide a sample project that can reproduce the problem.
@andrerfneves we'll merge as soon as it's ready 😉 it seems like we still have some problems with workspaces.
I'm not gonna be online tomorrow (Monday), so realistically I'll have time to finish this PR on Tuesday 👍

For now, you can always manually pass the provisioning profile mapping as described in the docs @andrerfneves

@KrauseFx Ah! wasn't aware I could do that. Sorry about that. And thanks for the hard work on such an awesome tool! Cheers

Without a sample project to reproduce the issue, I'm having a hard time fixing the problem 😢

I can provide one. Any reason not to merge the pull request I submitted, by the way? It doesn't conflict with this one

I've created the demo project. You can find it at https://github.com/igor-makarov/DemoWorkspaceWithXCConfig
Note that the issue I'm running into isn't related to workspaces. It's related to inherited .xccconfig files.
Also, my other issue (Iterating on the workspace can produce groups other than a project) that I've mentioned above, still needs to be addressed.

@igor-makarov Thanks for preparing the sample project, I gave it a try by changing the bundle identifier to my project one, and things are working without any problems.
[18:28:58]: Detected provisioning profile mapping: {"family.wwdc.app"=>"match AppStore family.wwdc.app"}
[18:28:58]: $ xcodebuild -list -workspace ./DemoWorkspaceWithXCConfig.xcworkspace
[18:28:59]: $ xcodebuild -showBuildSettings -workspace ./DemoWorkspaceWithXCConfig.xcworkspace -scheme DemoWorkspaceWithXCConfig

+-----------------------------------------------------+---------------------------------------------------------+
|                                            Summary for gym 2.38.1                                             |
+-----------------------------------------------------+---------------------------------------------------------+
| workspace                                           | ./DemoWorkspaceWithXCConfig.xcworkspace                 |
| export_options.provisioningProfiles.family.wwdc.app | match AppStore family.wwdc.app                          |
| scheme                                              | DemoWorkspaceWithXCConfig                               |
| destination                                         | generic/platform=iOS                                    |
| output_name                                         | DemoWorkspaceWithXCConfig                               |
| build_path                                          | /Users/fkrause/Library/Developer/Xcode/Archives/2017-0  |
|                                                     | 6-29                                                    |
| clean                                               | false                                                   |
| output_directory                                    | .                                                       |
| silent                                              | false                                                   |
| skip_package_ipa                                    | false                                                   |
| use_legacy_build_api                                | false                                                   |
| buildlog_path                                       | ~/Library/Logs/gym                                      |
| xcode_path                                          | /Applications/Xcode-9.app                               |
+-----------------------------------------------------+---------------------------------------------------------+

[18:29:00]: $ set -o pipefail && xcodebuild -workspace ./DemoWorkspaceWithXCConfig.xcworkspace -scheme DemoWorkspaceWithXCConfig -destination 'generic/platform=iOS' -archivePath /Users/fkrause/Library/Developer/Xcode/Archives/2017-06-29/DemoWorkspaceWithXCConfig\ 2017-06-29\ 18.29.00.xcarchive archive | tee /Users/fkrause/Library/Logs/gym/DemoWorkspaceWithXCConfig-DemoWorkspaceWithXCConfig.log | xcpretty
[18:29:01]: ▸ Building DemoWorkspaceWithXCConfig/DemoWorkspaceWithXCConfig [Release]
[18:29:01]: ▸ Check Dependencies
[18:29:01]: ▸ Linking DemoWorkspaceWithXCConfig
[18:29:02]: ▸ Compiling LaunchScreen.storyboard
[18:29:05]: ▸ Compiling Main.storyboard
[18:29:05]: ▸ Processing Info.plist
[18:29:05]: ▸ Generating 'DemoWorkspaceWithXCConfig.app.dSYM'
[18:29:05]: ▸ Touching DemoWorkspaceWithXCConfig.app
[18:29:10]: ▸ Signing /Users/fkrause/Library/Developer/Xcode/DerivedData/DemoWorkspaceWithXCConfig-cfpacxhsmflleecusxxxcgiuiega/Build/Intermediates.noindex/ArchiveIntermediates/DemoWorkspaceWithXCConfig/InstallationBuildProductsLocation/Applications/DemoWorkspaceWithXCConfig.app
[18:29:11]: ▸ Touching DemoWorkspaceWithXCConfig.app.dSYM
[18:29:11]: ▸ Archive Succeeded
[18:29:11]: Generated plist file with the following values:
[18:29:11]: ▸ -----------------------------------------
[18:29:11]: ▸ {
[18:29:11]: ▸   "provisioningProfiles": {
[18:29:11]: ▸     "family.wwdc.app": "match AppStore family.wwdc.app"
[18:29:11]: ▸   },
[18:29:11]: ▸   "method": "app-store"
[18:29:11]: ▸ }
[18:29:11]: ▸ -----------------------------------------
[18:29:11]: $ /usr/bin/xcrun /Users/fkrause/Developer/fastlane/gym/lib/assets/wrap_xcodebuild/xcbuild-safe.sh -exportArchive -exportOptionsPlist '/var/folders/8q/f0rt7xtj4pd8q6vgh_mk2p4c00dw80/T/gym_config20170629-19615-3ens69.plist' -archivePath /Users/fkrause/Library/Developer/Xcode/Archives/2017-06-29/DemoWorkspaceWithXCConfig\ 2017-06-29\ 18.29.00.xcarchive -exportPath '/var/folders/8q/f0rt7xtj4pd8q6vgh_mk2p4c00dw80/T/gym_output20170629-19615-1mhwq53'
[18:29:35]: Compressing 1 dSYM(s)
[18:29:35]: $ cd '/Users/fkrause/Library/Developer/Xcode/Archives/2017-06-29/DemoWorkspaceWithXCConfig 2017-06-29 18.29.00.xcarchive/dSYMs' && zip -r '/Users/fkrause/Developer/hacking/DemoWorkspaceWithXCConfig/DemoWorkspaceWithXCConfig.app.dSYM.zip' *.dSYM
[18:29:35]: ▸   adding: DemoWorkspaceWithXCConfig.app.dSYM/ (stored 0%)
[18:29:35]: ▸   adding: DemoWorkspaceWithXCConfig.app.dSYM/Contents/ (stored 0%)
[18:29:35]: ▸   adding: DemoWorkspaceWithXCConfig.app.dSYM/Contents/Info.plist (deflated 52%)
[18:29:35]: ▸   adding: DemoWorkspaceWithXCConfig.app.dSYM/Contents/Resources/ (stored 0%)
[18:29:35]: ▸   adding: DemoWorkspaceWithXCConfig.app.dSYM/Contents/Resources/DWARF/ (stored 0%)
[18:29:35]: ▸   adding: DemoWorkspaceWithXCConfig.app.dSYM/Contents/Resources/DWARF/DemoWorkspaceWithXCConfig (deflated 68%)

[18:29:35]: Successfully exported and compressed dSYM file
[18:29:35]: Successfully exported and signed the ipa file:
[18:29:35]: /Users/fkrause/Developer/hacking/DemoWorkspaceWithXCConfig/DemoWorkspaceWithXCConfig.ipa

I'll dive in further to figure out what it is.

Alright, seems like things work for me, we can do a follow-up patch if things end up not working

It seems that it matched the profiles from the match output, rather than from the project file.

FYI I still think it would fail under my config when not using match or the sigh patch I submitted. However I'm using sigh so my project builds ok.
So I guess I'm commenting just in case someone googles xcconfig in the future.

@KrauseFx I've noticed this still fails when either the provisioning profile or the app bundle ID are inherited from the project configuration (rather than being defined in the target) and are constructed using build setting $(VARIABLES) - this causes values to be missing from the  in the generated plist.
If I've not explained this well enough, please let me know and am happy to produce an example project tomorrow :). Cheers

@mjholgate This is my setup exactly and I already linked to a demo before.

Yes, I couldn't reproduce, if you let me know how I can reproduce the problem with the sample project, I can work on a fix 👍

@KrauseFx I've provided the sample project earlier.
The reason you haven't encountered the problem when reproducing is because you have match in place.


test: validate types in JS script tags

Enable type validation in plain JS script tags in tests.
Extracted from #3353

non-strict-template/#3353 is deliberate to not report errors, to make sure our codegen will not make TS errors reporting for JS script blocks by default.
Can you create tsconfig in a new directory under packages/vue-test-workspace/vue-tsc to enable checkJs and place the new test?

non-strict-template/#3353 is deliberate to not report errors, to make sure our codegen will not make TS errors reporting for JS script blocks by default.

Are you sure? Reading https://github.com/vuejs/language-tools/issues/3121 it appears to be about particular slot syntax not reporting errors rather than the whole template not reporting errors. With checkJs enabled and the fix for the original issue reverted it will still fail.

Maybe I didn't make it clear enough. We do want #3353/main.vue to not report an error because the same code in .js will also not report an error with checkJs disabled, and we want the script tag to behave consistently with .js.


Add differenceDirname optional argument

Add option to change diff file directory.
This option helps developers open the difference files with tools of their choice and organise the files as they choose.

Thanks! I already had a rework of how filepaths are handled in mind, so I folded this suggestion into that body of work. Available as options.diffsLocation in v2.0.0.


turn update pin to struct

Store the update in a struct to avoid re-obtaining for every set/reset

I think the TogglePin Trait is still useful since there is a mock and a non-mock TogglePin Trait, instead of a whole mock and non-moch ExecutionContext. Or did you mind something else?
This is not tested on Hardware yet but it SHOULD still work since it is just keeping the Pin inside of a struct. Because of the TogglePin trait, the rest of the code remains the same everywhere.

please test the thing on the actual hardware (specifically the update pin) as this was changed

Closes #71 and #78
Pls test on hardware