Allow expansion on "encrypted volumes"

Describe the bug
At present the expansion of volumes are blocked if the volume is encrypted. Considering expansion is a valid operation on an existing PVC, we should allow it eventhough volume is encrypted.
I am still not sure, we have to reencrypt the volume after expansion with the same key or we can use different key. But either way this operation can go through.
@vasyl-purchel any thoughts on this ?

I am still not sure, we have to reencrypt the volume after expansion with the same key or we can use different key. But either way this operation can go through.

This is handled automatically by LUKS during luksOpen -- it actually doesn't care about the block device size. If you are doing an online resize, just run cryptsetup resize and it will pick up the new size. This would be done before resizing any FS system.

Thanks for information. Considering we are doing online expansion we should be able to perform it with cryptsetup resize without hiccups.

Gone through the design of this once again and tried POC code which is working from the LUKS side, However I see some blockers to proceed further here with the complete solution.  That said, retrivel of the passphrase from the KMS becomes the bottleneck while expanding on node.
The nodeExpandRequest does not carry any 'secrets' with it [1] ( there is a request , but yet to get some consensus on this), We only have VolumeID in the request. We have to retrieve the rbdVol struct to get the KMS configuration. The genVolfromVolID() is the ONLY option, however this need cr to proceed further which is missing in the NodeExpand call. There is also no way to pass extra values in ControllerExpandResponse which  is limited to capacityBytes and NodeExpansionRequired boolean atm.
Either we have to attach KMS configuration to existing VolID or we have to wait for getting secret support in NodeExpand Call OR without secret in each call, we should have a mechanism to connect to the backend Cluster.
Thoughts  @vasyl-purchel @dillaman @ShyamsundarR @Madhu-1 ?
[1] https://github.com/container-storage-interface/spec/issues/445

Gone through the design of this once again and tried POC code which is working from the LUKS side, However I see some blockers to proceed further here with the complete solution. That said, retrivel of the passphrase from the KMS becomes the bottleneck while expanding on node.
The nodeExpandRequest does not carry any 'secrets' with it [1] ( there is a request , but yet to get some consensus on this), We only have VolumeID in the request. We have to retrieve the rbdVol struct to get the KMS configuration. The genVolfromVolID() is the ONLY option, however this need cr to proceed further which is missing in the NodeExpand call. There is also no way to pass extra values in ControllerExpandResponse which is limited to capacityBytes and NodeExpansionRequired boolean atm.
Either we have to attach KMS configuration to existing VolID or we have to wait for getting secret support in NodeExpand Call OR without secret in each call, we should have a mechanism to connect to the backend Cluster.
Thoughts @vasyl-purchel @dillaman @ShyamsundarR @Madhu-1 ?
[1] container-storage-interface/spec#445

@humblec what all the information you need for expanding the encrypted volume?

yes, in order to resize encrypted volume you need a passphrase and for that to be able to retrieve it - you need secrets... for vault as a kms - you don't need that as all configuration is in configmap (not secrets) and it uses service account to authenticate with vault 🤔
so the main problem is if your passphrase is stored in k8s secrets... storing it along the volume in metadata is not a good option - you put key to the volume near the volume itself... doesn't sound secure.
as a workaround you could add support only if vault kms is in use - then you can retrieve passphrase without secrets... but this will require quite a bit of rewrite of the code or "faking" secrets that can lead to pretty bad bugs...

yes, in order to resize encrypted volume you need a passphrase and for that to be able to retrieve it - you need secrets... for vault as a kms - you don't need that as all configuration is in configmap (not secrets) and it uses service account to authenticate with vault thinking

@vasyl-purchel  let me put some more info. We could make use of GetKMS(),  that pretty much read the required values for us, but currently GetKMS() need image attribute KMS ID as its arg,  which is missing in the NodeExpand. I was thinking that, if we could somehow get the KMSID we can use it (atleast) for Vault.

so the main problem is if your passphrase is stored in k8s secrets... storing it along the volume in metadata is not a good option - you put key to the volume near the volume itself... doesn't sound secure.

True.. Other option is placing kms ID in the metadata .

as a workaround you could add support only if vault kms is in use - then you can retrieve passphrase without secrets... but this will require quite a bit of rewrite of the code or "faking" secrets that can lead to pretty bad bugs...

Indeed.

Gone through the design of this once again and tried POC code which is working from the LUKS side, However I see some blockers to proceed further here with the complete solution. That said, retrivel of the passphrase from the KMS becomes the bottleneck while expanding on node.
The nodeExpandRequest does not carry any 'secrets' with it [1] ( there is a request , but yet to get some consensus on this), We only have VolumeID in the request. We have to retrieve the rbdVol struct to get the KMS configuration. The genVolfromVolID() is the ONLY option, however this need cr to proceed further which is missing in the NodeExpand call. There is also no way to pass extra values in ControllerExpandResponse which is limited to capacityBytes and NodeExpansionRequired boolean atm.
Either we have to attach KMS configuration to existing VolID or we have to wait for getting secret support in NodeExpand Call OR without secret in each call, we should have a mechanism to connect to the backend Cluster.

attaching kms configuration to a volumeID is not a good option.
we can enable resize of encrypted pvc if the vault is used as a KMS. we are already storing pieces of information related to the image on the staging path, we can add  encryptionKMSID to that. with this approach, we don't need a secret at all I believe.

Thoughts @vasyl-purchel @dillaman @ShyamsundarR @Madhu-1 ?
[1] container-storage-interface/spec#445

Why do you need the KMS to resize a volume? If I am reading the CSI spec correctly, since RBD is an online resizer, the LUKS device should already be opened (staged) by the time the NodeExpandVolume call arrives.

Why do you need the KMS to resize a volume? If I am reading the CSI spec correctly, since RBD is an online resizer, the LUKS device should already be opened (staged) by the time the NodeExpandVolume call arrives.

The device is opened and I could resize it with the POC code too. However I submitted hardcoded passphrase to this operation thinking that, the key is required for resize operation (https://github.com/ceph/ceph-csi/issues/1469#issuecomment-704815586) .
Also I see this for resize:
              If cryptsetup detected volume key for active device loaded in
              kernel keyring service, resize action would first try to
              retrieve the key using a token and only if it failed it'd ask
              for a passphrase to unlock a keyslot (LUKS) or to derive a
              volume key again (plain mode).  The kernel keyring is used by
              default for LUKS2 devices.

@dillman are you saying we dont need the key for resize operation is device is opened ? also regardless of LUKS version in use ?  I could try without submitting the passphrase though.

@dillman are you saying we dont need the key for resize operation if device is opened ? also regardless of LUKS version ( I was trying it with LUKS v1) in use ? I could try without submitting the passphrase though.

Since the size of the block device is not stored on disk, cryptsetup resize just instructs dm-crypt to notice the new size. There shouldn't be a need for the keys for that operation to succeed.

At least it seems to work on my laptop:
$ lsblk
NAME                                          MAJ:MIN RM   SIZE RO TYPE  MOUNTPOINT
nvme0n1                                       259:0    0 238.5G  0 disk
├─nvme0n1p1                                   259:1    0   200M  0 part  /boot/efi
├─nvme0n1p2                                   259:2    0     1G  0 part  /boot
└─nvme0n1p3                                   259:3    0 237.3G  0 part
  └─luks-4f8e89ee-1bf0-49eb-b437-94129895e578 253:0    0 237.3G  0 crypt
    ├─fedora-root                             253:1    0 229.4G  0 lvm   /
    └─fedora-swap                             253:2    0   7.9G  0 lvm   [SWAP]
$ cryptsetup resize luks-4f8e89ee-1bf0-49eb-b437-94129895e578

@dillaman I will check it out at my end too. Thanks !
One other reason for having credentials  ( not kms passphrase) in the nodeExpansion call was that, to derive rbdvol struct and thus knowing whether we are operating on an encrypted PV or not. I will look around some ways to hack it till we have that support in NodeExpansion request.

@dillaman I continued to experiment on this and here is what I see ( To summarize I do see a difference in behaviour depend on the LUKS version in use ( https://github.com/ceph/ceph-csi/issues/1469#issuecomment-705343204)
sh-4.4# lsblk
NAME                        MAJ:MIN RM  SIZE RO TYPE  MOUNTPOINT
sda                           8:0    0   50G  0 disk
|-sda1                        8:1    0    1G  0 part
`-sda2                        8:2    0   49G  0 part
  |-fedora_dhcp53--147-root 253:0    0   15G  0 lvm   /var/lib/kubelet/pods
  `-fedora_dhcp53--147-swap 253:1    0    5G  0 lvm
sdb                           8:16   0   30G  0 disk
`-ceph--6bc0bc0f--4813--4876--9604--e07fb56f3540-osd--data--b9692539--f353--4a1f--9b54--de40b293d64f
                            253:2    0   30G  0 lvm
sr0                          11:0    1 1024M  0 rom
rbd0                        252:0    0    1G  0 disk
`-luks-rbd-0001-0009-rook-ceph-0000000000000001-f94b99fd-f755-11ea-bfc4-ee9655eb3b57
                            253:3    0 1022M  0 crypt /var/lib/kubelet/pods/b5cb7fee-4dab-47e3-bed0-b1267806f72b/volumes/kubernetes.io~csi/pvc-f5b50d88-abee-46b1-a564-c8684a
rbd1                        252:16   0   10G  0 disk
`-luks-rbd-0001-0009-rook-ceph-0000000000000001-4439a7db-07ac-11eb-97bd-b2cd81afbed1
                            253:4    0   10G  0 crypt /var/lib/kubelet/pods/92961199-83ce-4383-a5b4-3aca40dc7102/volumes/kubernetes.io~csi/pvc-ea4ef28b-5e1e-4e81-9424-6e2c61

sh-4.4# cryptsetup resize luks-rbd-0001-0009-rook-ceph-0000000000000001-f94b99fd-f755-11ea-bfc4-ee9655eb3b57

sh-4.4# cryptsetup resize luks-rbd-0001-0009-rook-ceph-0000000000000001-4439a7db-07ac-11eb-97bd-b2cd81afbed1
**Enter passphrase for /dev/rbd1**: Error reading passphrase from terminal.

The interesting bit here is that :
 `luks-rbd-0001-0009-rook-ceph-0000000000000001-f94b99fd-f755-11ea-bfc4-ee9655eb3b57` is on **"LUKS v2"**
where
`luks-rbd-0001-0009-rook-ceph-0000000000000001-4439a7db-07ac-11eb-97bd-b2cd81afbed1` is on **LUKS v1"**

My cryptsetup version :** cryptsetup 2.2.2``**
Can you please share some more details about your setup wrt cryptsetup version and LUKS ?

@dillaman I will check it out at my end too. Thanks !
One other reason for having credentials ( not kms passphrase) in the nodeExpansion call was that, to derive rbdvol struct and thus knowing whether we are operating on an encrypted PV or not and may be other details too. I will look around some ways to hack it till we have that support in NodeExpansion request.

we already store the encrypted status in node staging https://github.com/ceph/ceph-csi/blob/0b5da6a12796cdf10e467b220e24d703e08cbb7f/internal/rbd/rbd_util.go#L1014

@dillaman I will check it out at my end too. Thanks !
One other reason for having credentials ( not kms passphrase) in the nodeExpansion call was that, to derive rbdvol struct and thus knowing whether we are operating on an encrypted PV or not and may be other details too. I will look around some ways to hack it till we have that support in NodeExpansion request.

we already store the encrypted status in node staging
https://github.com/ceph/ceph-csi/blob/0b5da6a12796cdf10e467b220e24d703e08cbb7f/internal/rbd/rbd_util.go#L1014

I know that, as I said, I was figuring out we require any extra details..etc .

@dillaman  Tried again with a new LUKs 2 device with cryptsetup (2.2.2) and it ask for the password.
rbd1                        252:16   0    1G  0 disk
`-luks-rbd-0001-0009-rook-ceph-0000000000000001-9ac3b2ae-09f9-11eb-81df-6eb6254150ec
                            253:4    0 1008M  0 crypt /var/lib/kubelet/pods/e185f10c-7424-4676-925f-f52767401bd7/volumes/kubernetes.io~csi/pvc-b377aa6e-c70d-42b8-82c6-dd1847

sh-4.4# cryptsetup resize luks-rbd-0001-0009-rook-ceph-0000000000000001-9ac3b2ae-09f9-11eb-81df-6eb6254150ec
Enter passphrase for /dev/rbd1: Error reading passphrase from terminal.

dm-crypt:
name:           dm_crypt vermagic:       5.3.7-301.fc31.x86_64 SMP mod_unload
May be I can try to upgrade cryptsetup , however would like to cross check with the versions in your setup

@humblec My kernel is 5.7.11-200.fc32.x86_64, my cryptsetup is cryptsetup-2.3.4-1.fc32.x86_64, and my device is LUKS1:
$ cryptsetup resize luks-4f8e89ee-1bf0-49eb-b437-94129895e578 --debug
# cryptsetup 2.3.4 processing "cryptsetup resize luks-4f8e89ee-1bf0-49eb-b437-94129895e578 --debug"
# Running command resize.
# Locking memory.
# Installing SIGINT/SIGTERM handler.
# Unblocking interruption on signal.
# Allocating crypt device context by device luks-4f8e89ee-1bf0-49eb-b437-94129895e578.
# Initialising device-mapper backend library.
# dm version   [ opencount flush ]   [16384] (*1)
# dm versions   [ opencount flush ]   [16384] (*1)
# Detected dm-ioctl version 4.42.0.
# Detected dm-crypt version 1.21.0.
# Device-mapper backend running with UDEV support enabled.
# dm status luks-4f8e89ee-1bf0-49eb-b437-94129895e578  [ opencount noflush ]   [16384] (*1)
# Releasing device-mapper backend.
# Trying to open and read device /dev/nvme0n1p3 with direct-io.
# Allocating context for crypt device /dev/nvme0n1p3.
# Trying to open and read device /dev/nvme0n1p3 with direct-io.
# Initialising device-mapper backend library.
# dm versions   [ opencount flush ]   [16384] (*1)
# dm table luks-4f8e89ee-1bf0-49eb-b437-94129895e578  [ opencount flush securedata ]   [16384] (*1)
# Trying to open and read device /dev/nvme0n1p3 with direct-io.
# dm versions   [ opencount flush ]   [16384] (*1)
# dm deps luks-4f8e89ee-1bf0-49eb-b437-94129895e578  [ opencount flush ]   [16384] (*1)
# Crypto backend (OpenSSL 1.1.1g FIPS  21 Apr 2020) initialized in cryptsetup library version 2.3.4.
# Detected kernel Linux 5.8.12-200.fc32.x86_64 x86_64.
# PBKDF pbkdf2-sha256, time_ms 2000 (iterations 0).
# Reading LUKS header of size 1024 from device /dev/nvme0n1p3
# Key length 64, device size 497606656 sectors, header size 4036 sectors.
# dm versions   [ opencount flush ]   [16384] (*1)
# dm table luks-4f8e89ee-1bf0-49eb-b437-94129895e578  [ opencount flush securedata ]   [16384] (*1)
# Trying to open and read device /dev/nvme0n1p3 with direct-io.
# Resizing device luks-4f8e89ee-1bf0-49eb-b437-94129895e578 to 0 sectors.
# dm versions   [ opencount flush ]   [16384] (*1)
# dm table luks-4f8e89ee-1bf0-49eb-b437-94129895e578  [ opencount flush securedata ]   [16384] (*1)
# Calculated device size is 497602560 sectors (RW), offset 4096.
# Device has already requested size 497602560 sectors.
# Releasing crypt device /dev/nvme0n1p3 context.
# Releasing device-mapper backend.
# Closing read only fd for /dev/nvme0n1p3.
# Unlocking memory.
Command successful.

@dillaman thanks! may be this is LUKS v2 requirement ?

You can also try to just store the passphrase in the kernel keyring while the device is mapped:
cryptsetup token add --key-description my:key --key-slot 0 /dev/rbdXYZ
echo -n "key" | keyctl padd user my:key @u
cryptsetup open --type luks --token-only ...

# when unmapping
keyctl unlink  %user:my:key @u

@dillaman thanks! may be this is LUKS v2 requirement ?

Yes, since LUKSv2 stores a json-encoded bit of metadata that includes a size parameter (which it needs to verify is set to dynamic). I suspect it would need to decrypt that data to verify that your LUKSv2 is properly configured for dynamic resizing (but dm-crypt itself wouldn't care otherwise).

@dillaman thanks! may be this is LUKS v2 requirement ?

Yes, since LUKSv2 stores a json-encoded bit of metadata that includes a size parameter (which it needs to verify is set to dynamic). I suspect it would need to decrypt that data to verify that your LUKSv2 is properly configured for dynamic resizing (but dm-crypt itself wouldn't care otherwise).

There we are :( .. back to the requirement of passphrase retrival for resize operation. Indeed as mentioned in previous comments in LUKS v2 it was asking for password. Also LUKS v2 looks to be the default now a days or atleast in RHEL ..etc, so we need to retrieve the passphrase for doing online expansion.

There we are :( .. back to the requirement of passphrase retrival for resize operation.

Why? If LUKSv2 is being used, store the passphrase in the kernel keyring on map.

Looks like cryptsetup >= v2.0.3 has a bug with cryptsetup resize [1]
[1] https://gitlab.com/cryptsetup/cryptsetup/-/issues/601

There we are :( .. back to the requirement of passphrase retrival for resize operation.

Why? If LUKSv2 is being used, store the passphrase in the kernel keyring on map.

I actually missed this update (https://github.com/ceph/ceph-csi/issues/1469#issuecomment-706151502) while composing my previous reply :)
I have to explore the kernel ring addition and retrival process. At the same time, I  find a way to get the required details to talk to the KMS and retrieve the password  with some additional details to rbdvol struct. So, may be I will go ahead with the latter one for now.

So, may be I will go ahead with the latter one for now.

Please don't store KMS settings in the metadata :pray:

So, may be I will go ahead with the latter one for now.

Please don't store KMS settings in the metadata pray

Sure, we dont need KMS settings embedded  in the metadata , rather just need one identifier or label which is KMS ID ( for ex: vault-test) which is the label to get the KMS settings later in the code path .. so we are good , however I have to experiment on this :)

Sure, we dont need KMS settings embedded in the metadata , rather just need one identifier or label which is KMS ID ( for ex: vault-test) which is the label to get the KMS settings later in the code path .. so we are good , however I have to experiment on this :)

.... or you wait for the proper fix and apply pressure to RHEL to get the fix included in the next Z stream.

Sure, we dont need KMS settings embedded in the metadata , rather just need one identifier or label which is KMS ID ( for ex: vault-test) which is the label to get the KMS settings later in the code path .. so we are good , however I have to experiment on this :)

.... or you wait for the proper fix and apply pressure to RHEL to get the fix included in the next Z stream.

oh.. yeah.. thanks for finding the bug/regression in cryptsetup and reporting it @dillaman ! Also thanks for other discussions here :+1:

I think that what @dillaman proposes with storing it in kernel ring might be a much better option to explore since you won't need to instantiate KMS which means less chances of an error and will work no matter what KMS is in use... i.e. it will work exactly the same for single passphrase from k8s secretes for example...

Can anyone please tell, why the encrypted volume is not able to expand?
I'm I missing some steps?
[root@linux-vm1 build]# rbd create --size 1G rbd-pool/image0
[root@linux-vm1 build]# rbd map rbd-pool/image0
/dev/rbd0
[root@linux-vm1 build]# cryptsetup luksFormat /dev/rbd0

WARNING!
========
This will overwrite data on /dev/rbd0 irrevocably.

Are you sure? (Type 'yes' in capital letters): YES
Enter passphrase for /dev/rbd0:
Verify passphrase:
[root@linux-vm1 build]# cryptsetup luksDump /dev/rbd0
LUKS header information
Version:        2
Epoch:          3
Metadata area:  16384 [bytes]
Keyslots area:  16744448 [bytes]
UUID:           28a5bd1f-1723-4770-98d4-7570680b7406
Label:          (no label)
Subsystem:      (no subsystem)
Flags:          (no flags)

Data segments:
  0: crypt
        offset: 16777216 [bytes]
        length: (whole device)
        cipher: aes-xts-plain64
        sector: 512 [bytes]

Keyslots:
  0: luks2
        Key:        512 bits
        Priority:   normal
        Cipher:     aes-xts-plain64
        Cipher key: 512 bits
        PBKDF:      argon2i
        Time cost:  6
        Memory:     1048576
        Threads:    4
        Salt:       9d 34 5f c7 2c 7b 92 d9 ec 4b 38 ca 53 3a f6 24
                    f3 87 5f 02 9c 54 80 61 61 72 e7 9a ac d6 72 e8
        AF stripes: 4000
        AF hash:    sha256
        Area offset:32768 [bytes]
        Area length:258048 [bytes]
        Digest ID:  0
Tokens:
Digests:
  0: pbkdf2
        Hash:       sha256
        Iterations: 129007
        Salt:       ec dc eb e2 11 31 13 f1 d6 a7 99 0b 42 4f c1 1c
                    ba a8 70 23 76 2c 72 0e c0 d4 93 b3 5c cd 5f 55
        Digest:     32 3d 21 ca 80 c6 0b f3 c5 fd b3 af 05 a1 f5 76
                    66 95 f7 a2 bc b1 c2 f5 f8 17 aa 91 01 b4 c3 67
[root@linux-vm1 build]# cryptsetup open /dev/rbd0 crypto-rbd0
Enter passphrase for /dev/rbd0:
[root@linux-vm1 build]# mkfs -t ext4 /dev/mapper/crypto-rbd0
mke2fs 1.45.6 (20-Mar-2020)
Creating filesystem with 258048 4k blocks and 64512 inodes
Filesystem UUID: cb5f1b0e-cc54-43cf-9ea3-21b085accd33
Superblock backups stored on blocks:
        32768, 98304, 163840, 229376

Allocating group tables: done
Writing inode tables: done
Creating journal (4096 blocks): done
Writing superblocks and filesystem accounting information: done

[root@linux-vm1 build]# mount /dev/mapper/crypto-rbd0 /mnt/test/
[root@linux-vm1 build]# df -Th
Filesystem              Type      Size  Used Avail Use% Mounted on
devtmpfs                devtmpfs  5.9G     0  5.9G   0% /dev
tmpfs                   tmpfs     5.9G     0  5.9G   0% /dev/shm
tmpfs                   tmpfs     2.4G  656K  2.4G   1% /run
/dev/mapper/fedora-root xfs       125G  9.2G  116G   8% /
/dev/vda1               xfs      1014M  312M  703M  31% /boot
/dev/vdb                xfs       100G   64G   37G  64% /data
tmpfs                   tmpfs     1.2G     0  1.2G   0% /run/user/0
/dev/mapper/crypto-rbd0 ext4      977M  2.5M  908M   1% /mnt/test
[root@linux-vm1 build]# rbd resize rbd-pool/image0 --size 2G
Resizing image: 100% complete...done.
[root@linux-vm1 build]# rbd info  rbd-pool/image0
rbd image 'image0':
        size 2 GiB in 512 objects
        order 22 (4 MiB objects)
        snapshot_count: 0
        id: 85cab8acf5a
        block_name_prefix: rbd_data.85cab8acf5a
        format: 2
        features: layering, exclusive-lock, object-map, fast-diff, deep-flatten
        op_features:
        flags:
        create_timestamp: Tue Jun 29 11:21:39 2021
        access_timestamp: Tue Jun 29 11:21:39 2021
        modify_timestamp: Tue Jun 29 11:21:39 2021
[root@linux-vm1 build]# resize2fs /dev/mapper/crypto-rbd0 2G
resize2fs 1.45.6 (20-Mar-2020)
The containing partition (or device) is only 258048 (4k) blocks.
You requested a new size of 524288 blocks.

[root@linux-vm1 build]# resize2fs /dev/rbd0 2G
resize2fs 1.45.6 (20-Mar-2020)
resize2fs: Device or resource busy while trying to open /dev/rbd0
Couldn't find valid filesystem superblock.
[root@linux-vm1 build]# df -Th
Filesystem              Type      Size  Used Avail Use% Mounted on
devtmpfs                devtmpfs  5.9G     0  5.9G   0% /dev
tmpfs                   tmpfs     5.9G     0  5.9G   0% /dev/shm
tmpfs                   tmpfs     2.4G  656K  2.4G   1% /run
/dev/mapper/fedora-root xfs       125G  9.2G  116G   8% /
/dev/vda1               xfs      1014M  312M  703M  31% /boot
/dev/vdb                xfs       100G   64G   37G  64% /data
tmpfs                   tmpfs     1.2G     0  1.2G   0% /run/user/0
/dev/mapper/crypto-rbd0 ext4      977M  2.5M  908M   1% /mnt/test
[root@linux-vm1 build]#

Thanks!

Please ignore, I think I got it now,
With the below steps, it works now:
[root@linux-vm1 build]# cryptsetup resize /dev/mapper/crypto-rbd0
Enter passphrase for /dev/rbd0:

[root@linux-vm1 build]# resize2fs /dev/mapper/crypto-rbd0
resize2fs 1.45.6 (20-Mar-2020)
Filesystem at /dev/mapper/crypto-rbd0 is mounted on /mnt/test; on-line resizing required
old_desc_blocks = 1, new_desc_blocks = 1
The filesystem on /dev/mapper/crypto-rbd0 is now 520192 (4k) blocks long.

[root@linux-vm1 build]# df -Th
Filesystem              Type      Size  Used Avail Use% Mounted on
devtmpfs                devtmpfs  5.9G     0  5.9G   0% /dev
tmpfs                   tmpfs     5.9G     0  5.9G   0% /dev/shm
tmpfs                   tmpfs     2.4G  656K  2.4G   1% /run
/dev/mapper/fedora-root xfs       125G  9.2G  116G   8% /
/dev/vda1               xfs      1014M  312M  703M  31% /boot
/dev/vdb                xfs       100G   64G   37G  64% /data
tmpfs                   tmpfs     1.2G     0  1.2G   0% /run/user/0
/dev/mapper/crypto-rbd0 ext4      2.0G  3.0M  1.9G   1% /mnt/test

Thanks!

For the curious readers:
The resize was working all fine with LUKS1 version too:
[root@linux-vm1 build]# rbd create --size 1G rbd-pool/image0
[root@linux-vm1 build]# rbd map rbd-pool/image0
/dev/rbd0
[root@linux-vm1 build]# cryptsetup luksFormat --type luks1 /dev/rbd0

WARNING!
========
This will overwrite data on /dev/rbd0 irrevocably.

Are you sure? (Type 'yes' in capital letters): YES
Enter passphrase for /dev/rbd0:
Verify passphrase:
[root@linux-vm1 build]# cryptsetup luksDump /dev/rbd0
LUKS header information for /dev/rbd0

Version:        1
Cipher name:    aes
Cipher mode:    xts-plain64
Hash spec:      sha256
Payload offset: 4096
MK bits:        512
MK digest:      c0 6d 6b ed 2d ef 07 16 9c 1b 5a 48 58 c6 40 e5 ee 13 03 4e
MK salt:        db d7 32 b6 b0 a3 69 62 f8 57 20 41 4a 1a 24 20
                12 6a cc cf c0 7b e5 bc d9 4a 92 ca 12 83 b5 d4
MK iterations:  126030
UUID:           7a90552a-2530-45bc-ad75-3777ebf5ec34

Key Slot 0: ENABLED
        Iterations:             2137768
        Salt:                   29 20 56 c2 10 13 2b eb a1 66 7a a3 51 5b d7 93
                                58 23 da b7 bc e0 4c 4f 6d 15 dc be 9d 5e b8 f5
        Key material offset:    8
        AF stripes:             4000
Key Slot 1: DISABLED
Key Slot 2: DISABLED
Key Slot 3: DISABLED
Key Slot 4: DISABLED
Key Slot 5: DISABLED
Key Slot 6: DISABLED
Key Slot 7: DISABLED
[root@linux-vm1 build]# cryptsetup open /dev/rbd0 crypto-rbd0
Enter passphrase for /dev/rbd0:
[root@linux-vm1 build]# mkfs -t ext4 /dev/mapper/crypto-rbd0
mke2fs 1.45.6 (20-Mar-2020)
Creating filesystem with 261632 4k blocks and 65408 inodes
Filesystem UUID: 8412a223-4f66-44b6-b63f-bbbf3bbb4a0e
Superblock backups stored on blocks:
        32768, 98304, 163840, 229376

Allocating group tables: done
Writing inode tables: done
Creating journal (4096 blocks): done
Writing superblocks and filesystem accounting information: done

[root@linux-vm1 build]# mount /dev/mapper/crypto-rbd0 /mnt/test/
[root@linux-vm1 build]# df -Th
Filesystem              Type      Size  Used Avail Use% Mounted on
devtmpfs                devtmpfs  5.9G     0  5.9G   0% /dev
tmpfs                   tmpfs     5.9G     0  5.9G   0% /dev/shm
tmpfs                   tmpfs     2.4G  656K  2.4G   1% /run
/dev/mapper/fedora-root xfs       125G  9.2G  116G   8% /
/dev/vda1               xfs      1014M  312M  703M  31% /boot
/dev/vdb                xfs       100G   64G   37G  64% /data
tmpfs                   tmpfs     1.2G     0  1.2G   0% /run/user/0
/dev/mapper/crypto-rbd0 ext4      990M  2.6M  921M   1% /mnt/test
[root@linux-vm1 build]# rbd resize rbd-pool/image0 --size 2G
Resizing image: 100% complete...done.
[root@linux-vm1 build]# cryptsetup resize /dev/mapper/crypto-rbd0
[root@linux-vm1 build]# resize2fs /dev/mapper/crypto-rbd0
resize2fs 1.45.6 (20-Mar-2020)
Filesystem at /dev/mapper/crypto-rbd0 is mounted on /mnt/test; on-line resizing required
old_desc_blocks = 1, new_desc_blocks = 1
The filesystem on /dev/mapper/crypto-rbd0 is now 523776 (4k) blocks long.
[root@linux-vm1 build]# df -Th
Filesystem              Type      Size  Used Avail Use% Mounted on
devtmpfs                devtmpfs  5.9G     0  5.9G   0% /dev
tmpfs                   tmpfs     5.9G     0  5.9G   0% /dev/shm
tmpfs                   tmpfs     2.4G  656K  2.4G   1% /run
/dev/mapper/fedora-root xfs       125G  9.2G  116G   8% /
/dev/vda1               xfs      1014M  312M  703M  31% /boot
/dev/vdb                xfs       100G   64G   37G  64% /data
tmpfs                   tmpfs     1.2G     0  1.2G   0% /run/user/0
/dev/mapper/crypto-rbd0 ext4      2.0G  3.0M  1.9G   1% /mnt/test
[root@linux-vm1 build]# cryptsetup luksDump /dev/rbd0
LUKS header information for /dev/rbd0

Version:        1
Cipher name:    aes
Cipher mode:    xts-plain64
Hash spec:      sha256
Payload offset: 4096
MK bits:        512
MK digest:      c0 6d 6b ed 2d ef 07 16 9c 1b 5a 48 58 c6 40 e5 ee 13 03 4e
MK salt:        db d7 32 b6 b0 a3 69 62 f8 57 20 41 4a 1a 24 20
                12 6a cc cf c0 7b e5 bc d9 4a 92 ca 12 83 b5 d4
MK iterations:  126030
UUID:           7a90552a-2530-45bc-ad75-3777ebf5ec34

Key Slot 0: ENABLED
        Iterations:             2137768
        Salt:                   29 20 56 c2 10 13 2b eb a1 66 7a a3 51 5b d7 93
                                58 23 da b7 bc e0 4c 4f 6d 15 dc be 9d 5e b8 f5
        Key material offset:    8
        AF stripes:             4000
Key Slot 1: DISABLED
Key Slot 2: DISABLED
Key Slot 3: DISABLED
Key Slot 4: DISABLED
Key Slot 5: DISABLED
Key Slot 6: DISABLED
Key Slot 7: DISABLED
[root@linux-vm1 build]#

Thanks!

BTW, here are my env details
[root@linux-vm1 ~]# cryptsetup --version
cryptsetup 2.3.5

[root@linux-vm1 ~]# uname -a
Linux linux-vm1 5.12.0+ #7 SMP Wed Apr 28 12:36:15 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

[root@linux-vm1 ~]# cat /etc/redhat-release
Fedora release 33 (Thirty Three)

@Madhu-1 @nixpanic @humblec @dillaman @vasyl-purchel
After performing the above experiment, for me, it looks like neither LUKS1 nor LUKS2 is involved in the unnecessary query, for the passphrase at the time of resizing while the crypt device is open, but it looks like cryptsetup tool version matters here.
Yes, as @dillaman also mentioned before, commit
Fix cryptsetup resize using LUKS2 tokens.

Fix a bug where cryptsetup needlessly asked for passphrase
even though volume key was already unlocked via LUKS2 token.

Fixes: #601.

https://gitlab.com/cryptsetup/cryptsetup/-/commit/a00722e80c714beb9557ea9ab4c61e39f8c7850b fixing https://gitlab.com/cryptsetup/cryptsetup/-/issues/601 is part of cryptsetup 2.3.5, and with cryptsetup 2.3.5 we do not hit this issue, check my previous comments for the command history.
If this is the case, can we live with adding a dependency to cryptsetup >= 2.3.5 at our docker files and install docs/guides for all the ceph-csi versions supporting encryption?
Or do you think we should hack around in ceph-csi to fix this bug in cryptsetup?
Thoughts?

@Madhu-1 @nixpanic @humblec @dillaman @vasyl-purchel
After performing the above experiment, for me, it looks like neither LUKS1 nor LUKS2 is involved in the unnecessary query, for the passphrase at the time of resizing while the crypt device is open, but it looks like cryptsetup tool version matters here.
Yes, as @dillaman also mentioned before, commit
Fix cryptsetup resize using LUKS2 tokens.

Fix a bug where cryptsetup needlessly asked for passphrase
even though volume key was already unlocked via LUKS2 token.

Fixes: #601.

https://gitlab.com/cryptsetup/cryptsetup/-/commit/a00722e80c714beb9557ea9ab4c61e39f8c7850b fixing https://gitlab.com/cryptsetup/cryptsetup/-/issues/601 is part of cryptsetup 2.3.5, and with cryptsetup 2.3.5 we do not hit this issue, check my previous comments for the command history.
If this is the case, can we live with adding a dependency to cryptsetup >= 2.3.5 at our docker files and install docs/guides for all the ceph-csi versions supporting encryption?
Or do you think we should hack around in ceph-csi to fix this bug in cryptsetup?
Thoughts?

Thanks @pkalever
From downstream ceph image, we have below crypto version
sh-4.4# cryptsetup --version
cryptsetup 2.3.3

From upstream docker image
docker run ceph/ceph:v16 cryptsetup --version
cryptsetup 2.3.3

if we can get 2.3.5 cryptsetup in the ceph base image we don't need to workaround. if we don't get it then we have to do it.

[You are too quick, I had a slightly edited version in the previous comment]

if we can get 2.3.5 cryptsetup in the ceph base image we don't need to workaround. if we don't get it then we have to do it.

Yes, @Madhu-1 that makes sense.
Maybe at ceph-csi we can defend on the cryptsetup version, if cryptsetup >= 2.3.5 use LUKS2 else use LUKS1 (as LUKS1 doesn't have this bug)?

[You are too quick, I had a slightly edited version in the previous comment]

if we can get 2.3.5 cryptsetup in the ceph base image we don't need to workaround. if we don't get it then we have to do it.

Yes, @Madhu-1 that makes sense.
Maybe at ceph-csi we can defend on the cryptsetup version, if cryptsetup >= 2.3.5 use LUKS2 else use LUKS1 (as LUKS1 doesn't have this bug)?

let see if we can get the cryptsetup updated in the ceph base image. if not we can have this workaround.

Apologies, I think I had jumped the gun :-(
I see with LUKS2 its prompting for the password on resize even with cryptsetup 2.3.5
[root@linux-vm1 build]# cryptsetup resize /dev/mapper/crypto-rbd0
Enter passphrase for /dev/rbd0:
[root@linux-vm1 build]# cryptsetup --version
cryptsetup 2.3.5

cryptsetup will always ask for a passphrase if the volume key is passed via kernel keyring (default for LUKS2)
LUKS1 devices don't use kernel keyring for volume key (backward compatibility)
LUKS2 devices use kernel keyring for volume key by default, but luckily users can choose to override the default by --disable-keyring option during cryptsetup open command.
From cryptsetup man pages:
 --disable-keyring
              Do not load volume key in kernel keyring and store it directly in the dm-crypt target instead.  This option is supported only for the LUKS2 format.

Does anyone have any objections to using --disable-keyring ?
Here is an experiment:
[root@linux-vm1 build]# rbd create --size 1024 rbd-pool/image0
[root@linux-vm1 build]# rbd map rbd-pool/image0
/dev/rbd0
[root@linux-vm1 build]# cryptsetup -q luksFormat --type luks2 --hash sha256 /dev/rbd0
Enter passphrase for /dev/rbd0:
[root@linux-vm1 build]# cryptsetup open /dev/rbd0 crypto-rbd0 --disable-keyring
Enter passphrase for /dev/rbd0:
[root@linux-vm1 build]# mkfs -t ext4 /dev/mapper/crypto-rbd0
mke2fs 1.45.6 (20-Mar-2020)
Creating filesystem with 258048 4k blocks and 64512 inodes
Filesystem UUID: fa681275-e1ce-40de-8fea-9c55946b84aa
Superblock backups stored on blocks:
	32768, 98304, 163840, 229376

Allocating group tables: done
Writing inode tables: done
Creating journal (4096 blocks): done
Writing superblocks and filesystem accounting information: done

[root@linux-vm1 build]# mount /dev/mapper/crypto-rbd0 /mnt/test/
[root@linux-vm1 build]# df -Th
Filesystem              Type      Size  Used Avail Use% Mounted on
devtmpfs                devtmpfs  5.9G     0  5.9G   0% /dev
tmpfs                   tmpfs     5.9G     0  5.9G   0% /dev/shm
tmpfs                   tmpfs     2.4G  728K  2.4G   1% /run
/dev/mapper/fedora-root xfs       125G  9.2G  116G   8% /
/dev/vda1               xfs      1014M  312M  703M  31% /boot
/dev/vdb                xfs       100G   69G   32G  69% /data
tmpfs                   tmpfs     1.2G     0  1.2G   0% /run/user/0
/dev/mapper/crypto-rbd0 ext4      977M  2.5M  908M   1% /mnt/test
[root@linux-vm1 build]# rbd resize rbd-pool/image0 --size 2G
Resizing image: 100% complete...done.
[root@linux-vm1 build]# cryptsetup resize /dev/mapper/crypto-rbd0
[root@linux-vm1 build]# echo $?
0
[root@linux-vm1 build]# resize2fs /dev/mapper/crypto-rbd0
resize2fs 1.45.6 (20-Mar-2020)
Filesystem at /dev/mapper/crypto-rbd0 is mounted on /mnt/test; on-line resizing required
old_desc_blocks = 1, new_desc_blocks = 1
The filesystem on /dev/mapper/crypto-rbd0 is now 520192 (4k) blocks long.

[root@linux-vm1 build]# df -Th
Filesystem              Type      Size  Used Avail Use% Mounted on
devtmpfs                devtmpfs  5.9G     0  5.9G   0% /dev
tmpfs                   tmpfs     5.9G     0  5.9G   0% /dev/shm
tmpfs                   tmpfs     2.4G  728K  2.4G   1% /run
/dev/mapper/fedora-root xfs       125G  9.2G  116G   8% /
/dev/vda1               xfs      1014M  312M  703M  31% /boot
/dev/vdb                xfs       100G   69G   32G  69% /data
tmpfs                   tmpfs     1.2G     0  1.2G   0% /run/user/0
/dev/mapper/crypto-rbd0 ext4      2.0G  3.0M  1.9G   1% /mnt/test
[root@linux-vm1 build]#

[root@linux-vm1 build]# cryptsetup status /dev/mapper/crypto-rbd0
/dev/mapper/crypto-rbd0 is active and is in use.
  type:    LUKS2
  cipher:  aes-xts-plain64
  keysize: 512 bits
  key location: dm-crypt
  device:  /dev/rbd0
  sector size:  512
  offset:  32768 sectors
  size:    4161536 sectors
  mode:    read/write

Thanks!


AddressSanitizer: heap-use-after-free in TCPClient::removeConnection

Describe the bug
While running my Drogon application with the AndressSanitizer enabled it will occasionally terminate my application because it detects the use of an object after it's been freed. This happens when the TcpClient::removeConnection() method is called. In my application the Drogon application has a websocket connection to a client where it sends events. In some scenarios the client abruptly terminates (e.g. forcefully disconnects) while the application is trying to send these events. This is detected by the application and it will attempt to "stop" the websocket connection and tear it down. This works 99.8% of the time but in some "racey" situations, it appears Drogon/Trantor is attempting to call TcpClient::removeConnection() after the TcpClient has already been destroyed.
I looked at the code and the only place I see TcpClient::removeConnection() being called is from the connection callback which is initialized in TcpClient::newConnection, e.g.
conn->setCloseCallback(std::bind(&TcpClient::removeConnection, this, _1));
What has me concerned is that conn is a shared pointer and that callback function is being initialized by a raw this pointer to the TcpClient instance. It seems plausible there could be a scenario where the TcpClient instance has already been destroyed by the time the conn close callback is called. This seems to be the scenario I am seeing.  I do see that TcpClient::~TcpClient destructor makes some effort to re-initialize the conn->setCloseCalllback() function but it's not clear if this is always being done within the scope/context of the destructor or scheduled to be done later in the event loop callback.
Is it possible to shed some light for the circumstances when TcpClient::removeConnection() should be called and why the raw this pointer being passed to the conn->setCloseCallback() method is not a potential problem? Again, I seem to see the issue more often when the Websocket client abruptly aborts the connection (e.g. no clean Websocket handshake to disconnect). How is the connection prevented from calling the TcpClient::removeConnection() method after it's been destroyed? Any insights or suggestions you could provide would be appreciated.
To Reproduce
More frequent when websocket client connect is abruptly disconnected by a client application.
Expected behavior
Don't call TcpClient::removeConnection() after TcpClient has been destroyed.
Screenshots
N/A
Desktop (please complete the following information):

OS: Linux

I've done some additional investigation into this bug and have verified, in fact, that there are situations where TcpClient::removeConncection() is being called from the connection callback on a TcpClient that has already been freed. In order to address this issue I propose the following patches:
diff --git a/trantor/net/TcpClient.cc b/trantor/net/TcpClient.cc
index cc409e6..73d8b66 100644
--- a/trantor/net/TcpClient.cc
+++ b/trantor/net/TcpClient.cc
@@ -82,25 +82,25 @@ TcpClient::~TcpClient()
     {
         std::lock_guard<std::mutex> lock(mutex_);
         conn = std::dynamic_pointer_cast<TcpConnectionImpl>(connection_);
-    }
-    if (conn)
-    {
-        assert(loop_ == conn->getLoop());
-        // TODO: not 100% safe, if we are in different thread
-        auto loop = loop_;
-        loop_->runInLoop([conn, loop]() {
-            conn->setCloseCallback([loop](const TcpConnectionPtr &connPtr) {
-                loop->queueInLoop([connPtr]() {
-                    static_cast<TcpConnectionImpl *>(connPtr.get())
-                        ->connectDestroyed();
+        if (conn)
+        {
+            assert(loop_ == conn->getLoop());
+            // TODO: not 100% safe, if we are in different thread
+            auto loop = loop_;
+            loop_->runInLoop([conn, loop]() {
+                conn->setCloseCallback([loop](const TcpConnectionPtr &connPtr) {
+                    loop->queueInLoop([connPtr]() {
+                        static_cast<TcpConnectionImpl *>(connPtr.get())
+                            ->connectDestroyed();
+                    });
                 });
             });
-        });
-        conn->forceClose();
-    }
-    else
-    {
-        connector_->stop();
+            conn->forceClose();
+        }
+        else
+        {
+            connector_->stop();
+        }
     }
 }

@@ -166,7 +166,23 @@ void TcpClient::newConnection(int sockfd)
     conn->setConnectionCallback(connectionCallback_);
     conn->setRecvMsgCallback(messageCallback_);
     conn->setWriteCompleteCallback(writeCompleteCallback_);
-    conn->setCloseCallback(std::bind(&TcpClient::removeConnection, this, _1));
+
+    std::weak_ptr<TcpClient> weakSelf(shared_from_this());
+    auto closeCb = std::function<void(const TcpConnectionPtr &)>([weakSelf](const TcpConnectionPtr & c) {
+        if ( auto self = weakSelf.lock() )
+        {
+            self->removeConnection(c);
+        }
+        // Else the TcpClient instance has already been destroyed
+        else
+        {
+            LOG_DEBUG << "TcpClient::removeConnection was skipped because TcpClient instanced already freed";
+            c->getLoop()->queueInLoop(
+                std::bind(&TcpConnectionImpl::connectDestroyed,
+                  std::dynamic_pointer_cast<TcpConnectionImpl>(c)));
+        }
+    });
+    conn->setCloseCallback(closeCb);
     {
         std::lock_guard<std::mutex> lock(mutex_);
         connection_ = conn;
diff --git a/trantor/net/TcpClient.h b/trantor/net/TcpClient.h
index a2fba76..1034be9 100644
--- a/trantor/net/TcpClient.h
+++ b/trantor/net/TcpClient.h
@@ -34,7 +34,7 @@ class SSLContext;
  * @brief This class represents a TCP client.
  *
  */
-class TRANTOR_EXPORT TcpClient : NonCopyable
+class TRANTOR_EXPORT TcpClient : NonCopyable, public std::enable_shared_from_this<TcpClient>
 {
   public:
     /**

The first problem in TcpClient::~TcpClient() is that it needs to hold on to the mutex protecting the connection while it is being used. Previously it was only momentarily locked while initially accessing it with the dynamic_pointer_cast(). I suspect concurrent access to the connection should be prevented within the scope of the TcpClient.
The second problem was devising a mechanism to prevent the connection's setCloseCb() from calling TcpClient::removeConnection() on an already deleted TcpClient instance. This is fixed by publicly inheriting TcpClient from std::enable_shared_from_this<TcpClient> and then creating a std::function<> that takes a weak pointer to the TcpClient and in the callback tries to lock and convert this weak pointer to a shared pointer. If this fails then the TcpClient instance has already been destroyed and the same clean-up code in TcpClient::removeConnection() is attempted on the connection in the callback. If the TcpClient instance is still alive then TcpClient::removeConnection() is called directly.
During this investigation and testing I discovered another bug in trantor/net/EventLoop.cc. Bascially, in the destructor EventLoop::~EventLoop() the assert assert(!looping_) was being triggered.  The problem is the call to quit() in the destructor can execute quickly but the loop() function might take some time to notice and exit the while loop (setting looping_ = false). There is a small window here where the destructor's assert might be checked before the loop() function exits. My patch, although a bit kludgey, seems to work based on the assumption that EventLoop::loop() will eventually exit (fairly quickly) when quit_ = false. I simply loop waiting for this to occur in the EventLoop destructor instead of immediately asserting after the call to quit(). Also, since it seems two or more threads must be routinely calling methods on EventLoop I made the variables looping_ and quit_ volatile so the compiler wouldn't optimize out any reads from these variables. Since we don't care about retrieving a consistent (bool) value from looping_ and quit_ these are not made atomic types (std::atomic). We just need to eventually detect a consistent state change whether or not the change is temporarily inconsistent while the memory location of the bool is updated. Below is my patch:
diff --git a/trantor/net/EventLoop.cc b/trantor/net/EventLoop.cc
index 8a60a03..94e8a53 100644
--- a/trantor/net/EventLoop.cc
+++ b/trantor/net/EventLoop.cc
@@ -112,7 +112,17 @@ void EventLoop::resetAfterFork()
 EventLoop::~EventLoop()
 {
     quit();
-    assert(!looping_);
+
+    // Spin waiting for the loop to exit because
+    // this can take some time to complete.
+    // We could alternatively sleep (and block) this
+    // thread for a short time if
+    // looping_ == true and then re-check.
+    // If it still hasn't exited then one option
+    // might be to abort the program.
+    // assert(!looping_);
+    while ( looping_ );
+
     t_loopInThisThread = nullptr;
 #ifdef __linux__
     close(wakeupFd_);
diff --git a/trantor/net/EventLoop.h b/trantor/net/EventLoop.h
index d28f6b8..1278726 100644
--- a/trantor/net/EventLoop.h
+++ b/trantor/net/EventLoop.h
@@ -287,9 +287,9 @@ class TRANTOR_EXPORT EventLoop : NonCopyable
     void abortNotInLoopThread();
     void wakeup();
     void wakeupRead();
-    bool looping_;
+    volatile bool looping_;
     std::thread::id threadId_;
-    bool quit_;
+    volatile bool quit_;
     std::unique_ptr<Poller> poller_;

     ChannelList activeChannels_;

I hope you have the opportunity to consider these changes. It's highly likely you want to implement these changes differently or realize that other (related) changes should also be made as a result of my patches. I'm interested in any feedback you can offer and I hope you'll be able to integrate these changes (in some form) quickly into the master branch. Please let me know if you have any questions about my implementation.
Thanks for maintaining and making this framework available to all!

@gschmottlach-xse Thanks so much for your investigation. Would you like to make a PR to the trantor repo?

I have made two PR's for the trantor repo. One PR addresses the TCPClient issue with removeConnection() being called after the TcpClient has already been destroyed. The second PR has the EventLoop destructor fix so it waits (indefinitely) for the loop to exit in the destructor instead of immediately asserting. Please review and make any changes you deem necessary. I look forward to these PRs being merged into Trantor/Drogon master.
Thanks!

@gschmottlach-xse Thanks so much for your patch. I'll take some time to review.

Any progress towards reviewing and integrating my Trantor PR and ultimately pulling it into Drogon?

Sorry for the delay merging of https://github.com/an-tao/trantor/pull/197, please resolve the compilation error on windows of https://github.com/an-tao/trantor/pull/198. Thanks so much.


Applying textedits from code actions doesn't automatically adjust tabs automagically

VSCode Version: 1.15-insiders 9b6cc9f774f424b4e8a91d4a2d3d4ad8efb9cfb9
OS Version: macOS Sierra 10.12.6

Steps to Reproduce:

Open a text file using spaces for tabs (doesn't matter how many indented lines there are)
Have a code completion response returning tabs for indentation, like:

Result: {
    "label": "something() : void",
    "kind": 3,
    "detail": "Override method in 'IFoo'",
    "documentation": " This should be inherited ",
    "sortText": "999998843",
    "filterText": "something",
    "insertText": "something",
    "insertTextFormat": 2,
    "textEdit": {
        "range": {
            "start": {
                "line": 9,
                "character": 4
            },
            "end": {
                "line": 9,
                "character": 5
            }
        },
        "newText": "@Override\npublic void something() {\n\t${0}\n}"
    }

Once the text edit applied, the indentation is transformed to use spaces

Do the same thing from a code action, like

[Trace - 3:59:58 PM] Received response 'textDocument/codeAction - (436)' in 9ms.
Result: [
    {
        "title": "Add unimplemented methods",
        "command": "java.apply.workspaceEdit",
        "arguments": [
            {
                "changes": {
                    "file:///Users/fbricon/Dev/souk/vertx-game-server/src/main/java/com/redhat/middleware/keynote/discovery/Meh.java": [
                        {
                            "range": {
                                "start": {
                                    "line": 7,
                                    "character": 12
                                },
                                "end": {
                                    "line": 7,
                                    "character": 12
                                }
                            },
                            "newText": "\n\n\t"
                        },
                        {
                            "range": {
                                "start": {
                                    "line": 7,
                                    "character": 12
                                },
                                "end": {
                                    "line": 7,
                                    "character": 12
                                }
                            },
                            "newText": "@Override\n\tpublic void something() {\n\t\t"
                        },
                        {
                            "range": {
                                "start": {
                                    "line": 7,
                                    "character": 12
                                },
                                "end": {
                                    "line": 7,
                                    "character": 12
                                }
                            },
                            "newText": "\n\t}"
                        }
                    ]
                }
            }
        ]
    }
]

in that case, the indentations are inserted as tabs, while I expect the automagical conversion to spaces.
I'm not sure if this is caused by the code action returning multiple text edits.
Reproduces without extensions: Maybe.
This can be reproduced by testing https://github.com/eclipse/eclipse.jdt.ls/pull/322. cc @aeschli

That conversion of \t to spaces or tab characters is a feature specific to snippets. Snippets can be written by users in a .json file or contributed from extensions via a .json file, i.e. without any source code.
Since all code actions come from a smart language server, they do not get the same treatment. The smart language server knows about the editor settings w.r.t. tabs and spaces, e.g. for formatting. IMHO it would be unexpected if \t in code actions would be massaged in any way.

ATM there are no plans to change this


Revert "FIX / Workflow: Fix Mac-OS CI issues"

As discussed offline @BenjaminBossan

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.


Plugin breaks my feed

When this plugin is installed and enabled, my feed disappears entirely.
See screenshot.

The configuration is space is also blank. (see screenshot)

There are also console errors relating to the extension.
Lastly, there is a console error that shows up on the main feed page as well.

Plugin was recently installed so this should be the newest version
freshrss version 1.22.0

Hi, I'm very sorry for this inconvenience. I didn't stumble upon this while development. I'll take a look on it and report back when the issue is fixed. Thank you for letting me know!


Lint and test errors on npm install in the react-codemod directory.

npm ERR! Failed at the<EMAIL_ADDRESS>prepublish script 'npm run lint && npm run test'.
It seems to be showing lots of lint errors?

FYI This was running under windows 10.
I had also accidentally added react-codemod as a subdirectory of my own project, instead of directly within the github folder.
Moving it up a level, I do still get 70 warnings, but install is not showing errors.
So I'm closing this.


Inconsistent results

Platform
npm using wappalyzer package ^6.9.14
Describe the bug
When I run the test multiple times in a row I get different results.
To Reproduce
I run the wappalyzer scan (using this example code: https://github.com/AliasIO/wappalyzer/tree/master/src/drivers/npm) with the following options:
const options = {
  delay: 1000,
  maxDepth: 3,
  maxUrls: 10,
  maxWait: 10000,
  recursive: true,
  probe: true,
  noScripts: false
};

Expected behavior
I would expect to get the same result when I run in a few times in a row

Hey @ejerskov , as in request #5559 ,
there can be many reasons why the results differ.
If I'm not mistaken, the extension analyzes the current page. If you try to open several pages of the same site, you may notice that the number of technologies will gradually increase
npm, on the contrary, recursively looks at several pages at once. All this can be changed in the settings. Sometimes you can get 1-2 technologies like Cloudflare and the protection tool will not allow you to analyze the technologies on this site


chore: remove toy vm files

Now that lookups and permutations are included within the core avm code, we do not need to test lookups via the TOY prover, its inclusion is bloat and gives the crypto team more to do when refactoring

#5326  👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Maddiaa0 and the rest of your teammates on  Graphite

Benchmark results
Metrics with a significant change:

note_trial_decrypting_time_in_ms (32): 97.3 (+185%)
note_trial_decrypting_time_in_ms (64): 132 (+124%)

Detailed results
All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call  to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.
This benchmark source data is available in JSON format on S3 here.
Values are compared against data from master at commit 5f0f4fca and shown if the difference exceeds 1%.
L2 block published to L1
Each column represents the number of txs on an L2 block published to L1.

Metric
8 txs
32 txs
64 txs

l1_rollup_calldata_size_in_bytes
5,668
18,820
36,356

l1_rollup_calldata_gas
66,088
238,852
469,676

l1_rollup_execution_gas
647,186
929,210
1,305,856

l2_block_processing_time_in_ms
1,275 (-7%)
4,554 (-7%)
9,058 (-5%)

note_successful_decrypting_time_in_ms
189 (+11%)
578 (+12%)
1,001 (-3%)

note_trial_decrypting_time_in_ms
92.0 (+20%)
:warning: 97.3 (+185%)
:warning: 132 (+124%)

l2_block_building_time_in_ms
18,153 (-1%)
68,680 (-2%)
136,307

l2_block_rollup_simulation_time_in_ms
8,136 (-1%)
28,883 (-4%)
56,827

l2_block_public_tx_process_time_in_ms
9,994 (-1%)
39,742
79,382

L2 chain processing
Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric
5 blocks
10 blocks

node_history_sync_time_in_ms
13,871 (-4%)
26,066 (-2%)

note_history_successful_decrypting_time_in_ms
1,250 (+3%)
2,493 (+2%)

note_history_trial_decrypting_time_in_ms
82.2 (+30%)
139 (-16%)

node_database_size_in_bytes
18,645,072
35,070,032

pxe_database_size_in_bytes
29,859
59,414

Circuits stats
Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit
circuit_simulation_time_in_ms
circuit_input_size_in_bytes
circuit_output_size_in_bytes

private-kernel-init
277 (-2%)
44,366
28,244

private-kernel-ordering
211 (-1%)
52,868
14,326

base-parity
1,787 (-4%)
128
311

base-rollup
704 (-3%)
165,787
925

root-parity
1,598 (-1%)
1,244
311

root-rollup
51.1 (-1%)
4,487
725

private-kernel-inner
636 (-1%)
73,771
28,244

public-kernel-app-logic
455 (+2%)
35,260
28,215

public-kernel-tail
176 (+2%)
40,926
28,215

merge-rollup
8.20 (-2%)
2,696
925

Tree insertion stats
The duration to insert a fixed batch of leaves into each tree type.

Metric
1 leaves
16 leaves
64 leaves
128 leaves
512 leaves
1024 leaves
2048 leaves
4096 leaves
32 leaves

batch_insert_into_append_only_tree_16_depth_ms
9.84 (-2%)
15.6 (-3%)
N/A
N/A
N/A
N/A
N/A
N/A
N/A

batch_insert_into_append_only_tree_16_depth_hash_count
16.8
31.6
N/A
N/A
N/A
N/A
N/A
N/A
N/A

batch_insert_into_append_only_tree_16_depth_hash_ms
0.574 (-2%)
0.484 (-2%)
N/A
N/A
N/A
N/A
N/A
N/A
N/A

batch_insert_into_append_only_tree_32_depth_ms
N/A
N/A
44.9 (-2%)
71.6 (-1%)
231
430 (-4%)
871
1,731 (-1%)
N/A

batch_insert_into_append_only_tree_32_depth_hash_count
N/A
N/A
96.0
159
543
1,055
2,079
4,127
N/A

batch_insert_into_append_only_tree_32_depth_hash_ms
N/A
N/A
0.461 (-2%)
0.442 (-1%)
0.421
0.403 (-3%)
0.414
0.414
N/A

batch_insert_into_indexed_tree_20_depth_ms
N/A
N/A
52.9 (-2%)
106 (-1%)
339
647 (-4%)
1,316 (-1%)
2,628
N/A

batch_insert_into_indexed_tree_20_depth_hash_count
N/A
N/A
104
207
691
1,363
2,707
5,395
N/A

batch_insert_into_indexed_tree_20_depth_hash_ms
N/A
N/A
0.471 (-2%)
0.477 (-1%)
0.459
0.447 (-3%)
0.456
0.455
N/A

batch_insert_into_indexed_tree_40_depth_ms
N/A
N/A
N/A
N/A
N/A
N/A
N/A
N/A
60.6 (-1%)

batch_insert_into_indexed_tree_40_depth_hash_count
N/A
N/A
N/A
N/A
N/A
N/A
N/A
N/A
109

batch_insert_into_indexed_tree_40_depth_hash_ms
N/A
N/A
N/A
N/A
N/A
N/A
N/A
N/A
0.531 (-1%)

Miscellaneous
Transaction sizes based on how many contract classes are registered in the tx.

Metric
0 registered classes

tx_size_in_bytes
22,012

Transaction processing duration by data writes.

Metric
0 new note hashes
1 new note hashes

tx_pxe_processing_time_ms
3,150 (-2%)
1,732 (-1%)

Metric
0 public data writes
1 public data writes

tx_sequencer_processing_time_ms
11.1 (-19%)
1,264 (+2%)


BaseRestSerializer should handle timestamp headers consistently. fixes #625

I took a crack at fixing #625, adding test cases for the failing case as well as the cases that were working before.

Excellent, thanks for the PR.


Cannot change renderer

I am trying to use a custom renderer, but i can't seem to make svelte-preprocess-markdown to actually use it. I tried following the instructions for getting the renderer in the README. My rollup.config.js looks like this:
import svelte from 'rollup-plugin-svelte';
import resolve from 'rollup-plugin-node-resolve';
import commonjs from 'rollup-plugin-commonjs';
import livereload from 'rollup-plugin-livereload';
import { terser } from 'rollup-plugin-terser';
import rollup_start_dev from './rollup_start_dev';
import {markdown, Renderer} from 'svelte-preprocess-markdown';

const renderer = Renderer();

renderer.hr = function() { return 'myhr' }

const production = !process.env.ROLLUP_WATCH;

export default {
	input: 'src/main.js',
	output: {
		sourcemap: true,
		format: 'iife',
		name: 'app',
		file: 'public/bundle.js'
	},
	plugins: [
		svelte({
			extensions: ['.svelte', '.md'],
			preprocess: markdown({
				renderer,
			}),
			// enable run-time checks when not in production
			dev: !production,
			// we'll extract any component CSS out into
			// a separate file — better for performance
			css: css => {
				css.write('public/bundle.css');
			}
		}),

		// If you have external dependencies installed from
		// npm, you'll most likely need these plugins. In
		// some cases you'll need additional configuration —
		// consult the documentation for details:
		// https://github.com/rollup/rollup-plugin-commonjs
		resolve({
			browser: true,
			dedupe: importee => importee === 'svelte' || importee.startsWith('svelte/')
		}),
		commonjs(),

		// In dev mode, call `npm run start:dev` once
		// the bundle has been generated
		!production && rollup_start_dev,

		// Watch the `public` directory and refresh the
		// browser on changes when not in production
		!production && livereload('public'),

		// If we're building for production (npm run build
		// instead of npm run dev), minify
		production && terser()
	],
	watch: {
		clearScreen: false
	}
};

But my --- sections still render as <hr> tags.

Yeah, looks like the renderer option do nothing. I'll check tomorow.

I've fixed it. (https://github.com/AlexxNB/svelte-preprocess-markdown/commit/6d770a8a844dd447fda81151fa198416d245ab03) New version already published on NPM.

Works great. Thank you!

I know this issue was closed in November, but I am having trouble getting this to work as well. My config is exactly as @mattiash initial post above but I am still not able to get this to render on any pages, .svelte or .md.

Will check. There was huge Marked update in last commites. Maybe getting renderer has been broken.

Just done installation default Svelte template with svelte-preprocess markdown and use config above. Everything works as a charm. Also added the test to check renderer function.


ESM support

Description
ESM Support
Type of change

[x] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?
Test Environment:

OS: Windows & macOS
@graphql-codegen/...: latest
NodeJS: v14.17.0 & v16.3.0

Checklist:

[x] I have followed the CONTRIBUTING doc and the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[ ] Any dependent changes have been merged and published in downstream modules

Another issue found in @graphql-codegen/plugin-helpers https://github.com/dotansimha/graphql-code-generator/blob/master/packages/utils/plugins-helpers/src/resolve-external-module-and-fn.ts#L7
That will most definitely break in ESM

I found an issue in CLI, and it's the usage of "require.resolve", which is not supported in ESM https://github.com/dotansimha/graphql-code-generator/blob/master/packages/graphql-codegen-cli/src/config.ts#L183

Yeah, seems to be an issue. And we do use this in codegen core to load plugins, configurations, and loaders.  I guess we need a wrapper for that to make sure it's still supported in both setups.

Another issue found in @graphql-codegen/plugin-helpers https://github.com/dotansimha/graphql-code-generator/blob/master/packages/utils/plugins-helpers/src/resolve-external-module-and-fn.ts#L7
That will most definitely break in ESM

How do you think we should address this one?
cc @ardatan

How do you think we should address this one?

If that question is for me (maybe it wasn't), I feel like that is completely out of my scope since I'm not completely familiar with all the codegen internals, but adding support for esm I'd say is pretty much required if for example GraphQL.js starts adding support for ESM with the exports field, that will trigger duplicated graphql module versions, since if a commonjs module requires graphql, it will use the cjs version, and an esm module will use the esm version

How do you think we should address this one?

If that question is for me (maybe it wasn't), I feel like that is completely out of my scope since I'm not completely familiar with all the codegen internals, but adding support for esm I'd say is pretty much required for programmatic usage if for example GraphQL.js starts adding support for ESM with the exports field, that will trigger duplicated graphql module versions error, since if a commonjs module requires graphql, it will use the cjs version, and an esm module will use the esm version (this applies for every GraphQL library btw)

Yeah, the question was for you :D
I see what you mean, so I think maybe we can wait a bit longer on this PR, and plan this for the next major version.

@dotansimha @ardatan
While working in a example using graphql-codegen programmatically I realized that the plugin-helpers .mjs file is bundling requires that come from bundling change-case-all, in ESM world bundling dependencies most of the time is forbidden, because it will bundle the requires in the esm files, which of course breaks.
Of course the solution is to add "change-case-all" as a dependency in this specific package, but this applies to every package that intends to support esm

After adding the test-esm.mjs script (and locally fixing the pending to be merged fix of graphql-tools https://github.com/ardatan/graphql-tools/pull/3289) these errors appear, this script is only able to catch errors in the imports, not the logic inside of the scripts
https://user-images.githubusercontent.com/8672915/127908337-ac08b4be-7db2-45e9-aaf0-4b7c6642394d.mp4
I will take a look into fixing these issues

After https://github.com/ardatan/graphql-tools/pull/3300 is merged and publish, this PR should be ready @dotansimha

createRequireFromPath is already removed in Node.js v16 (it was already deprecated in earlier versions), I will have to remove it and only use "createRequire"

@dotansimha It doesn't allow me to request a review in the PR for some reason, but this PR should be ready


DEVICE_EXPOSURE Table - Ability to map devices

I am currently mapping a dataset with associated physical devices, it could be beneficial to be able to represent such concepts.
Additionally, the UK OHDSI OMOP group is currently talking about 'device' data.
https://ohdsi.github.io/CommonDataModel/cdm54.html#DEVICE_EXPOSURE

the field "device_type_concept_id" also need to come back to


Disabling alerts

When the markus tab is kept open for a long period of time, an alert pops up and interrupts whatever is going on. There should be an option where the alert could be turned off.

Do you mean the alert that says that the session is about to end?  (We added that so TAs would not add annotations or marks when the session was ended and their work would be lost when the page was refreshed to tell them that they need to log in again.)
If the markus tab does not have focus, does the alert interrupt the currently open tab?
What browser are you using?
Thanks.

It interrupts whatever I am doing whether I'm using the browser or not. I'm using chrome. Also my suggestion was not to disable it completely but have the option to disable it.

Could this be an alternative method of notifying the user?

I don't think notification support is good enough to be a valid alternative.

For reference, the browser support table for notifications is located here.


Set/change current view date

I am using calendarRange, and am programmatically updating the selection, and that works just fine.  However, sometimes that selection is not visible in the current view (e.g. looking at June, and the selection is the first week of May).  How can I update the current month to go to, say, the start date of the selection?
I've tried this, but it doesn't work (only snippets shown, for brevity):
    <calendar-range
      :events="calendarEvents"
      :selection="calendarSelection"
      :initialCurrent="currentCalendarDate"
    ></calendar-range>

    currentCalendarDate: function () {
      return this.currentCalendarMoment.toDate()
    },

    this.currentCalendarMoment = moment('2017-05-01')

there is  a errlog Error in render function: "Error: Cannot find module './zh/index.js'."

I think the reason is here.  ‘src/mixin.js’

The specific code location is here

default property

locale:{
type:String,
default: (navigator.language || navigator.userLanguage).substr(0,2).toUpperCase()
validator: function(value) { 				return value === value.toUpperCase(); 			}
}


Fix the command in section "Check the HTTP response in your logs" https://docs.influxdata.com/influxdb/v1.7/troubleshooting/frequently-asked-questions/#check-the-http-response-in-your-logs

https://docs.influxdata.com/influxdb/v1.7/troubleshooting/frequently-asked-questions/#check-the-http-response-in-your-logs
version 1.7.6-c1.7.6
The command should be
journald-ctl -u influxdb.service
I tried the command in the document and it didn't work.  When I use the command above it shows the logs.

Sorry the command should be
journalctl -u influxdb.service
and not
journald-ctl -u influxdb.service


src: cleanup the run timeout loop and check

:broom: cleans up the run timeout loop and error

Adds the timeout reached to a typed error on timeout, and factors out the liveness check to a function to avoid tail-stacking defer statements in the check loop.

I really like using verbose option. I tested and make test still works OK with macos.
/lgtm

The tests are failing because dapr cannot be installed for some reason.

/approve
/lgtm

The tests are failing because dapr cannot be installed for some reason.

After I amended the PR the tests passed (including dapr installation). I don't know why.


Site-wide crawl or leverage sitemap.xml file

Feature Request
There are a few other tools that either do site-wide scans or just allow you to plug in the sitemap.xml file into it.
Best examples of this I've found of this are:

https://github.com/MSU-NatSci/DomainAccessibilityAudit
https://github.com/GovTechSG/purple-hats

Rather than defining pages individually in a a11y.js file would be nice to just have one you could plug in and centrally maintain for SEO, etc.

I think this is still a page specific implementation.
I think this is an engine similar to axe or htmlcs rather than pa11y. Right?


fix again and not in the wrong way

Overview

PR Checklist

[ ] Related to designs:
[ ] Related to JIRA ticket: [ABC-123]
[ ] I have run this code to verify it works
[ ] This PR includes unit tests for the code change
[ ] This PR includes testing instructions tests for the code change
[ ] The alpha package of this PR is passing end-to-end tests in all relevant Codecademy repositories

Testing Instructions

Don't make me tap the sign.
PR Links and Envs

Repository
PR Link
PR Env

Monolith
Monolith PR
Monolith Env

Portal
Portal Link
Portal Env

Another Repo
Another Link
Another Env

🚀 Styleguide deploy preview ready!https://675c5ba6288e053253849e34--gamut-preview.netlify.appDeploy Logs


Allow diff between current open file's changes and the saved original

Sometimes I forget what changes I've done between saves and that would be a useful feature.

/duplicate of #26339


FreeRTOS+TCP Adding the combined driver for SAM4E and SAME70

Description
As the title says, this PR is adding a driver for 2 platforms: SAM4E and SAME70.
The driver makes use of the module ../Common/phyHandler.c, which initialises and polls the Ethernet PHY.
Related Issue
A user on the FreeRTOS forum  asked for this driver. He had found a reference on SourceForge, but the ZIP file had been deleted.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

About the last commit:
The function usGenerateProtocolChecksum() now needs a parameter that indicates the length of a packet.
The function xNetworkInterfaceOutput() called a function called xCheckLoopback(0, which is not defined. Added this parameter to vGMACGenerateChecksum() as well.

/bot run checks

/bot run checks

/bot run checks

/bot run checks

The EMAC task was started with a fixed high task-priority. In other drivers a macro is defined:
  #define niEMAC_HANDLER_TASK_PRIORITY     configMAX_PRIORITIES - 1

Users can override this macro in FreeRTOSIPConfig.h.
A second change: The driver didn't work on SAME70 Rev-B. The reason was that mentioned part has 6 priority queues in its EMAC.
Those queues are also initialised now:
    /* Note that SAME70 REV B had 6 priority queues. */
    gmac_set_tx_priority_queue(p_hw, (uint32_t)&gs_tx_desc_null, GMAC_QUE_3);
    gmac_set_tx_priority_queue(p_hw, (uint32_t)&gs_tx_desc_null, GMAC_QUE_4);
    gmac_set_tx_priority_queue(p_hw, (uint32_t)&gs_tx_desc_null, GMAC_QUE_5);

The changes are tested on both SAME70 Rev-A and Rev-B.
Hein

About the commit "phyHandling.c : Make the right choice between slow and frequent polling of the Ethernet PHY"
When the PHY Link Status is low, one wants to poll the status frequently. But once the PHY is connected, frequent polling of the PHY would be disturbing, and a slow polling is preferred.
In order to check the Link Status, the mask was compared with a const phyBMSR_LINK_STATUS, which is incorrect, mentioned macro is a register bit.
-    if( ( pxPhyObject->ulLinkStatusMask & phyBMSR_LINK_STATUS ) != 0 )
+    if( ( pxPhyObject->ulLinkStatusMask & ( ulBitMask >> 1 ) ) != 0 )

Closing this PR #57 because a new PR #78 (v2) will take it over.
Hein


Fix language tags in dataset cards

Fix language tags in all dataset cards, so that they are validated (aligned with our languages.json resource).

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

The non-passing tests are caused by other missing information in the dataset cards.


new nginx default.conf to serve 404 page

https://app.clubhouse.io/featurepeek/story/4146/serve-404-page-without-redirecting


Fix: dashboard redirect url for oauth providers

fixes the redirect url for oauth providers

I added some changeset to the PR.


[i2c] Loopback test

During D1 review, I've been asked to add loopback test to the IP. Would the following match the expectations?

LOOPBACK bit will be asserted and provided to the IP through the register interface to perform the loopback test.
In the host mode, the IP will retrieve the data from fmt_fifo, remove the formatting bits and deposit the data into rx_fifo. The outputs scl_o and sda_o will be released. So, the data will travel from and back to TLUL.
In the target mode and upon receiving a write transaction from an agent, the IP will write the data into acq_fifo, retrieve the data from it, remove formatting bits and deposit the data into tx_fifo for transmission back to the agent. So, the data will travel from and back to the bus.

@igk8 for the first case, do you literally mean go from fmt_fifo into rx_fifo? I think ideally it would go onto the lines that would have exited the module and then loop from there back to the incoming line (kind of like the system loopback in uart), is that what you mean?
The second case sounds good to me!

@tjaychen @igk8 first it would go to the sending data state in which scl and sda are originally driven. However, in loopback, we will disable driving scl_o and sda_o and route data back to receiving data state (also disable receiving data from scl and sda)

do you think there's any downside to just allowing it to go to sclk_o and
sda_o and loop that directly back to scl_i / sda_i?
Maybe in the end it doesn't make a huge difference, but it would be nice to
loopback as much as possible.
On Wed, Mar 24, 2021 at 9:42 AM Tung Hoang @.***> wrote:

@tjaychen https://github.com/tjaychen @igk8 https://github.com/igk8
first it would go to the sending data state in which scl and sda are
originally driven. However, in loopback, we will disable driving scl_o and
sda_o and route data back to receiving data state (also disable receiving
data from scl and sda)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lowRISC/opentitan/issues/5753#issuecomment-805982121,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAH2RSVTQ72XMCLBJT6HEB3TFIJBDANCNFSM4ZVZ3LPQ
.

@tjaychen the downside is we can not examine all states of fsm (e.g. states that drive scd_o/scl_o and receive scl_i/sda_i) but in that case, dut can do loopback itself regardless of the target devices.
if you want to loopback outside the i2c top, we need either modifying testbench or require i2c_agent support which means that dut is not able to support loopback itself

i'm sorry, i did not mean outside of i2c top.
i meant something like what uart is doing.
https://github.com/lowRISC/opentitan/blob/master/hw/ip/uart/rtl/uart_core.sv#L250
Basically looping back at the last point, and not going from one fifo
directly to another.
Does that make sense? Maybe that's what you were saying already.
On Wed, Mar 24, 2021 at 6:57 PM Tung Hoang @.***> wrote:

@tjaychen https://github.com/tjaychen the downside is we can not
examine all states of fsm (e.g. states that drive scd_o/scl_o and receive
scl_i/sda_i) but in that case, dut can do loopback itself regardless of the
target devices.
if you want to loopback outside the i2c top, we need either modifying
testbench or require i2c_agent support which means that dut is not able to
support loopback itself
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lowRISC/opentitan/issues/5753#issuecomment-806302929,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAH2RSUNHCVNB6V2LHMP67DTFKKCFANCNFSM4ZVZ3LPQ
.

i'm sorry, i did not mean outside of i2c top. i meant something like what uart is doing. https://github.com/lowRISC/opentitan/blob/master/hw/ip/uart/rtl/uart_core.sv#L250 Basically looping back at the last point, and not going from one fifo directly to another. Does that make sense? Maybe that's what you were saying already.
@tjaychen thanks, it makes sense

This implies that the IP (in either host or target mode) must be able to transmit serial data and receive it concurrently. This is what UART is doing.
Is there a real need to enable multi-host features (host-host clock synchronization and host bus arbitration)? The bus arbitration also requires the host to transmit and receive data concurrently.

@tjaychen Thank you! The "line" loopback test mode is implemented in PR #5964.

just wanted to double check, is this one of the last features for I2C? I think almost everything else is in?

Yes, this is one of the last features for I2C, probably the last one. Unless we decide to support multi-host features at some point.
From: tjaychen @.>
Reply-To: lowRISC/opentitan @.>
Date: Wednesday, April 7, 2021 at 2:25 PM
To: lowRISC/opentitan @.>
Cc: Igor Kouznetsov @.>, Mention @.***>
Subject: Re: [lowRISC/opentitan] [i2c] Loopback test (#5753)
CAUTION: This email originated from outside of Western Digital. Do not click on links or open attachments unless you recognize the sender and know that the content is safe.
just wanted to double check, is this one of the last features for I2C? I think almost everything else is in?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/lowRISC/opentitan/issues/5753#issuecomment-815275219, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AM3U3KQ5QWNXJSVCHR3IS23THTEVFANCNFSM4ZVZ3LPQ.

thanks @igk8 .
Feel free to close this issue once the loopback function is merged.

@igk8 just checking in again on this one.
If it's done, let's close the issue and officially advance i2c to d2.

Loopback function is merged, closing this issue.


Adding word-wrap support

Hi,
I'm trying to add the wrapping funciton to avoid lines without line-numbers
You can see the result here :
http://jsfiddle.net/sLLwtwbL/2/
When resizing the window, the gutter is updated.
It resolves the defect #4 and #13
Please add the following style in your css sheet:
.hljs-line{   display: inline; }

Your jsfiddle doesn't seem to work on Chrome/Edge. Firefox works fine.


Feature Request: Album art on media key preview

When pressing the media key to skip to the next track there is a preview in the top right corner showing the track name and the app icon. Instead of using the app icon it should display the album art.

Maybe the image could be extracted from the HTML class="sc-artwork sc-artwork-placeholder-9 image__full g-opacity-transition"
I would attempt this myself but I am not very familiar with JavaScript.

soundcloud.js
module.exports = class SoundCloud extends Events {
  constructor(window) {
    super()
    this.window = window

    var x = window.document.getElementsByClassName("sc-artwork image__full g-opacity-transition")[0].getAttribute("style");
    var pos1 = x.indexOf("url(") + 4;
    var pos2 = x.indexOf(");") - pos1;
    var image = x.substr(pos1, pos2);

    window.on('page-title-updated', debounce((_, title) => {
      this.onTitleUpdated(_, title, image)
    }), titleDebounceWaitMs)
    fixFlakyMediaKeys(window)
  }

main.js
soundcloud.on('play', (title, subtitle, image) => {
    mainWindow.webContents.send('notification', title, subtitle, image)
  })

I'm far from a JavaScript/node/electron expert and I couldn't build the project on my machine but perhaps something like this would work?

Was going for something like this:

Also relevant:

@kinglime thanks for the idea and the investigation, definitely sounds like a nice feature. Will add it some time in the future. Also related to #61


Release version v0.9.0

What does this PR do?:

Release changes for AI Software Templates v0.9.0.
Which issue(s) this PR fixes:

https://issues.redhat.com/browse/DEVAI-185
PR acceptance criteria:
Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened and linked to this PR, if they are not in the PR scope due to various constraints.

[ ] Tested and Verified

[X] Documentation (READMEs, Product Docs, Blogs, Education Modules, etc.)

How to test changes / Special notes to the reviewer:

Should we pull the change from here into this PR? @michael-valdron https://github.com/redhat-ai-dev/ai-lab-app/pull/36

Should we pull the change from here into this PR? @michael-valdron redhat-ai-dev/ai-lab-app#36

https://github.com/redhat-ai-dev/ai-lab-template/pull/46

We should add the LICENSE file (Apache 2) to the release branch.
(It needs to be added to main as well)

@johnmcollier  We'll need to open PRs into all the same repositories to first target main, I can rebase the outstanding release PRs and create new ones to merge in the changes into those I've already merged.

We should add the LICENSE file (Apache 2) to the release branch.
(It needs to be added to main as well)

#47


The meaning of  gt_true_bbox and gt_bbox

The meaning of  gt_true_bbox and gt_bbox in the code

The gt_true_bbox means the original coco bounding box annotations of the objects (the real GT).
The gt_bbox means：generate the qc point first，use the qc point as the center and use the fixed number like the 16*16 as the weight and height to generate the pseudo gt bbox, which is convenient for mmdetection dataset loader.  In P2BNet，the center of the gt_bbox is received as the qc point for following use.


chore: Add Dependabot for dependency updates

I'm not familiar with Dependabot - what does this bring?

I'm not familiar with Dependabot - what does this bring?

Currently it already submits PRs for security issues like https://github.com/tabatkins/bikeshed/pull/1851
This will start running it on a schedule and include updates for both the Python dependencies and GitHub Actions tasks
https://github.blog/2020-06-01-keep-all-your-packages-up-to-date-with-dependabot/

Currently it already submits PRs for security issues like https://github.com/tabatkins/bikeshed/pull/1851
This will start running it on a schedule and include updates for both the Python dependencies and GitHub Actions tasks
https://github.blog/2020-06-01-keep-all-your-packages-up-to-date-with-dependabot/

Ah yeah seems reasonable, and now that we have even better CI, let's pull the trigger.

Ah yeah seems reasonable, and now that we have even better CI, let's pull the trigger.


Feature/edit and delete events

Added ability to edit events (if you own the event or are an admin)
Added ability to delete events (if you own the event or are an admin)
Added pagination to the events page, there are now 5 events shown per page

LGBT
only the log may be necessary


[community] Added SentenceWindowRetriever

[ ] PR title: "package: description"

Added appropriate title

Description

Adds  a new type of retriever called Sentence Window Retriever

Also adds a modification to TextSplitter to help implement the retriever

[ ] Add tests and docs: If you're adding a new integration, please include
No tests added yet. Let me know if any specific tests are required.
Plan to add documentation on how to run the retriever

[ ] Lint and test: Tests ran successfully

@eyurtsev So I have modified the implementation of SWR to be datastore agnostic.
I went with approach of defining a get_document_by_ids function at the vectorstores which enables a common method for querying vectorstore based on IDs.
One of the issues with the implementation of SWR is that the search functionality is not standardized across vectorstores.
Chroma : has similarity_search_by_vector and 'similarity_search_by_vector_with_score'
Pinecone : does not have similarity_search_by_vector but only 'similarity_search_by_vector_with_score'
Milvus : has similarity_search_by_vector but instead of  'similarity_search_by_vector_with_score' has 'similarity_search_with_score_by_vector' which is probably a typo
I can work around the the different search method names for now, but it might be helpful for Pinecone to also have a 'similarity_search_by_vector' implementation and for Milvus to use the same standardized function names like the other vectorstores.

@eyurtsev @efriis Could I get a review on this?
Let me know if I need to add any additional details to explain the changes made.
I did have a question on how to include the langchain_pinecone as a dependency within community. The unit tests throw an error when I import PineconeVectorStore from langchain_pinecone

@eyurtsev @efriis Can I get a review on this?

@eyurtsev @efriis @hwchase17 @baskaryan Can I get a review on this?

@rsk2327 You'll need to standby for ~1 month. We'll be focusing on the vectorstore abstractions after the 0.2 release.
The main things so far:

Addition of  get_documents_by_ids to the base abstraction
Potentially addition of an id attribute on a document (so the ID is not randomly in the metadata).

Text splitters:

Determine what if any kind of metadata we should be propagating in the text splitter for provenance purposes

I'll leave some comments in the PR itself as well

closing as stale

+1 on this PR


DevOps: Backend Pipeline to AWS ECS

Currently blocked: AWS account locked due to "suspicious activity"


Fix two issues

Lumping these two together because they are both trivial and the overhead of doing separate PRs seems unwarranted.
The two issues are:

When flip() is applied to footprints that have eg: *.Cu set as a layer, we would force the component to one side, breaking its multi-layer-ness.  I've resolved this here by explicitly checking for *.
Module allowed building footprints containing polygons, but not loading them.  This sort of footprint is generatable via https://github.com/mtl/svg2mod.  I've resolved this by adding the polygon parser.

Thanks!


Suggestion: Add a progress bar

There's quite a bit on unused space to the right of the counter. Maybe it could be filled with an ASCII progress bar?
(Also, is it possible to hide the days and hours etc. when they aren't used for a counter? They make it more difficult to visually grep what's going on.)

I'm not against putting a progress bar into snore because it's not complex and add only a few SLOC. I just wonder if it's a real need or just a cool feature to have. I may decide to implement this when I'll have some free time, in the while feel free to send a patch.
You cannot toggle what snore prints without changing the source code. We may hide all the fields with zero value, this would improve things up for you?

Hrm. I'd say it's needed to make snore really useful,as it provides a much-needed visual cue as to how much time as passed.
Ah, yeah - that would greatly improve things and cut away some of the visual noise.
I think if either were to be implemented, they should be behind flags though.

I've been playing around with the code and discovered that adding command line arguments needs to refactor main(). After that we need to collects time pieces (arguments) and loop on them, instead of simply loop on argv. Alternatively, we could make a nozerofields compile-time option.
There is also a glitch from 0 to 0.99 seconds where no fields are shown at all: the only output is " | ". We could hide only fields with zero precision but this sounds a bit like a workaround, not a solution.
For the progress bar it's only matter of a single function but we also need that command line arguments handling gets properly implemented. Since I'm not much interested in this, it's likely that I will not implement it myself but patches are welcome.
All of this may introduce more complexity than I would like to. I'll think about it for a while.


Ensure that PROTOC_INCLUDES Can Be Correctly Identified When PROTOC Is a Symbolic Link

Previously, the location of PROTOC_INCLUDES was incorrectly (that is, not at all) detected when PROTOC was an absolute path to a symbolic link.

I notice that there are a bunch of other uses of 'which' within the compiler scripts. Should those be updated as well?

I don't think so since 'which' is generally portable as-is and those use cases don't seem to warrant readlink or readlink -f.


Nav bar fix

Minor margin and padding fixes in navbar. And reformed the burger menu now its easy to add any animation you would like.

Everything seems fine to me thank for your help @makssmeinart