Allow plugins to hook into the IPython console

Hi,
I'm writing a plugin to integrate our (in PySide2/PyQt5) written GUI into Spyder. For this some new features would be nice:
I need to communicate between the gui and the executing ipython kernel. For this I set a startup file to monkeypatch the interaction functions and send spyder messages using get_ipython().kernel.send_spyder_msg("my_msg_type",None, data). I don't see a clean way for a plugin to execute code on a kernel start. But I think this would be a nice feature.
Then I catch these messages by monkeypatching the NameSpaceBrowser._handle_spyder_msg function inside the register_plugin function.
This is kind of messy, it would be nice if I could just implement handlers for my message types.
Did I miss anything which would have made life easier? ;)
If there is interest in merging these features I would be willing to implement them :)
Best
Christopher

Did I miss anything which would have made life easier? ;)

No, you didn't. We don't provide a public API to interface with the IPython console yet, so I understand things like the ones you're trying to do are really hard.

If there is interest in merging these features I would be willing to implement them

Of course! But you need to do it for Spyder 4 (i.e. against our master branch) because we are not going to implement more new features for Spyder 3.
Please take a look at our api module:
https://github.com/spyder-ide/spyder/tree/master/spyder/api
add an ipythonconsole.py module there and define public functions that allow to extend the spyder_msg functionality.


Hidden KML features still incur hit detection performance costs (2D Mode)

Hidden KML features still go through a significant amount of the Openlayers hit detection stack. This includes getting all the way to a costing getImageData call on each hidden feature that shouldn't happen.
It's exclusive to KML layers due to how we manage their shown/hidden state on an individual feature level (and we never actually disable the overall layer visibility).

I don't think this has anything to do with overall layer visibility. What happens to hidden features on other sources?

I think when layers are invisible, they don't make it through to the portion of the hit detection doing the slow, unnecessary calls. This is likely a problem for hidden features in other sources as well, it's just more readily apparent with KML.

OpenLayers still hit detects features skipped by ol.PluggableMap#skipFeature. I thought I had fixed that problem with this mixin... I'll look into it.

Ok so this seems to be an OpenLayers bug in their skipped feature handling. If the feature is skipped, they shouldn't handle the END_GEOMETRY instruction which is making the getImageData call. They should just move on to the next feature like they do when the feature doesn't intersect the hit extent. I'll open an issue with them.


Wait before capturing a snapshot

Context 🕵♀
While attempting to capture a view that includes AsyncImage or revealing a component after a specified duration, it's been noticed that the resulting snapshot doesn't include images loaded via URL or views that appear with a slight delay. Even when using .snapshot(delay: 1.0), there's no noticeable improvement in this scenario.
What 🌱
To overcome this challenge, it would be helpful to incorporate an option to wait before taking the snapshot. This way, we can ensure that the snapshot accounts for the asynchronous loading of images and views that emerge after a delay.
Example of a view that isn't properly captured in the snapshot:
struct DummyView: View {
    @State var isButtonHidden = true
    var body: some View {
        VStack {
            Text("Some text")
            Text("Some text with delay")
                .opacity(isButtonHidden ? 0 : 1)

        }.onAppear {
            DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) {
                self.isButtonHidden = false
            }
        }
    }
}

It seems that this is a problem of the swift-snapshot-tests library. I found a PR in it that should fix this.
I'll try to think later about how to get around this inside the Prefire.

Also, it seems that .preferredColorScheme(.dark) is not handled properly. I found a PR that states it should already work 🤔 using swift-snapshot-tests . Do you think it could be the same cause?

It sounds like a bug in the Prefire. I set myself the task to fix it in the 2.0 release 😊


BTable info

Hi, I'm trying to use the BTable in my project but some functions I used before no longer work.
Like @row-clicked, @row-selected, @sort-changed, filter, busy and all the props to set classes to various thead / tbody / tfoot.
Looking at the documentation I only see indications for the BTableSimple.
Are these features missing? Do you have any idea when these changes will be released for the BTable?

@drakaena Many events have not been implemented yet, have a look at the issue #138 for more details.

There is no timeline atm. Perhaps once I'm done with finishing up the current tests, I will make this a priority. It seems to be something a fair few request.

@drakaena most of these feature was added lately https://github.com/cdmoro/bootstrap-vue-3/pull/624, https://github.com/cdmoro/bootstrap-vue-3/pull/616, https://github.com/cdmoro/bootstrap-vue-3/pull/609, and https://github.com/cdmoro/bootstrap-vue-3/pull/594
enjoy it.

I assume most additions are implemented, though the documentation on how to use them is not complete. I am going to close the issue


Fix for libraries being built with features support

@mbed-bot: TEST
HOST_OSES=windows
BUILD_TOOLCHAINS=GCC_ARM,ARM,IAR
TARGETS=K64F,KL25Z

👍


Replace references to issue numbers from our original bug tracker

Currently, there are many references to our original bug tracker. For example, inside the bugs directory, subdirectories are named by their issue number. We should give them more descriptive names.
In general, whenever an issue from our original bug tracker is referenced, we should replace this occurrence with its description.

Done by @PeterMatula in bcbab21ad57d084136ce5ffc8a1d6325c2cf792b.


Missing update command - documentation incorrect.

This page says that you can update an existing acl, however this does not work.  There is no az storage fs access update  command.  There is one for update-recursive, which does work recursively, but there is not one for updating a single acl.

Document Details
⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

ID: 74d1720a-2f9e-8f7d-1a4c-db08252e4aac
Version Independent ID: 3becacf7-661f-6c61-f466-5bfd48aa8706
Content: Use Azure CLI to manage ACLs in Azure Data Lake Storage Gen2 - Azure Storage
Content Source: articles/storage/blobs/data-lake-storage-acl-cli.md
Service: storage
Sub-service: data-lake-storage-gen2
GitHub Login: @jimmart-dev
Microsoft Alias: jammart

Using the set command as the update section shows replaces the entire acl and does not allow for updating a single acl.  It is a destructive command that replaces the entire acl.

@bigdatamoore
Thanks for your feedback! We will investigate and update as appropriate.

@bigdatamoore
I'm going to assign this to the document author so they can take a look at it accordingly.
@jimmart-dev
Can you please check and add your comments on this doc update request as applicable.

Thanks for your contribution to our documentation
Unfortunately, we have been unable to review this issue in a timely manner. We sincerely apologize for the delayed response. We are closing this issue. If you feel that the problem persists, please respond to this issue with additional information.
?
Please continue to provide feedback about the documentation. We appreciate your contributions to our community.
#please-close


BMI V&V

BMI V&V
Description

Category: v&v run
JIRA issue: MIC-3773

Changes and notes
Remove treatment components, which includes LDL-C and SBP multipliers that adjust GBD medicated exposures to unmedicated exposures.
Verification and Testing
Ran simulation for a few time steps.

Instead of commenting the code out, I would recommend:

Remove the code if not needed
Use feature flag to turn on/off the functionality associated with this commented code based on the use-case.
This helps keep the code clean and live and does not become stale.
Commented code becomes stale and creates confusion over period of time.

@ramittal This is a special case because we're only commenting out these lines for this specific run, and the code will be uncommented otherwise. I've made a bmi-develop branch that was checked out from main which I'll merge into. Its only purpose is to perform this run.


Send commit object in http response header for archives

fix #17834
demo
url=http://localhost:3000/asdfasdf/asdfasdf/archive/master.zip

curl --verbose --header 'X-Commit-Object: 1' --output /dev/null $url 2>&1 | grep X-Commit-Object
> X-Commit-Object: 1
< X-Commit-Object: dHJlZSAyZTkwMGI5YTFhOTllZWM3YzlmMDFlN2QxMTNmNDg4YWQyOGU0NmVmCmF1dGhvciBhc2RmYXNkZiA8YXNkZkBsb2NhbGhvc3QubG9jYWxkb21haW4+IDE2Mzg3OTkyNzggKzAxMDAKY29tbWl0dGVyIGFzZGZhc2RmIDxhc2RmQGxvY2FsaG9zdC5sb2NhbGRvbWFpbj4gMTYzODc5OTI3OCArMDEwMAoKSW5pdGlhbCBjb21taXQK

echo -n dHJlZSAyZTkwMGI5YTFhOTllZWM3YzlmMDFlN2QxMTNmNDg4YWQyOGU0NmVmCmF1dGhvciBhc2RmYXNkZiA8YXNkZkBsb2NhbGhvc3QubG9jYWxkb21haW4+IDE2Mzg3OTkyNzggKzAxMDAKY29tbWl0dGVyIGFzZGZhc2RmIDxhc2RmQGxvY2FsaG9zdC5sb2NhbGRvbWFpbj4gMTYzODc5OTI3OCArMDEwMAoKSW5pdGlhbCBjb21taXQK | base64 -d
tree 2e900b9a1a99eec7c9f01e7d113f488ad28e46ef
author asdfasdf<EMAIL_ADDRESS>1638799278 +0100
committer asdfasdf<EMAIL_ADDRESS>1638799278 +0100

Initial commit

You need to run make fmt

Why not just send the sha instead?

Why not just send the sha instead?

trust no one, thats why : )
i want to verify the archive by its commit hash

Codecov Report

Merging #17923 (03b1a45) into main (dcdb487) will increase coverage by 0.00%.
The diff coverage is 61.11%.

@@           Coverage Diff           @@
##             main   #17923   +/-   ##
=======================================
  Coverage   45.33%   45.34%
=======================================
  Files         815      815
  Lines       90719    90741   +22
=======================================
+ Hits        41131    41144   +13
- Misses      43020    43024    +4
- Partials     6568     6573    +5

Impacted Files
Coverage Δ

models/repo_archiver.go
44.44% <ø> (ø)

modules/git/repo_commit.go
47.00% <60.00%> (+0.28%)
:arrow_up:

routers/web/repo/repo.go
33.33% <61.53%> (+1.15%)
:arrow_up:

modules/queue/queue_channel.go
95.00% <0.00%> (-1.67%)
:arrow_down:

routers/web/repo/compare.go
44.27% <0.00%> (-0.36%)
:arrow_down:

routers/install/install.go
0.56% <0.00%> (-0.01%)
:arrow_down:

modules/queue/workerpool.go
50.00% <0.00%> (+0.76%)
:arrow_up:

services/repository/archiver/archiver.go
64.16% <0.00%> (+1.15%)
:arrow_up:

modules/setting/database.go
62.50% <0.00%> (+1.27%)
:arrow_up:

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 dcdb487...03b1a45. Read the comment docs.

It has been stale for long time and doesn't get interest or update. I will close it.
You are always welcome if there could be a open and friendly discussion.


add a run.py file or similar to actually run the analysis for a specific region/country

became somewhat obsolete now DamageScanner has become a class object within the core file.


Preparing for supporting array type for PostgreSQL

Added two parameters for yii\db\pgsql\ColumnSchema:
ColumnSchema::$dimension
ColumnSchema::$delimiter

Related with #7813 and #7481

Any progress on this one?

It depends on #7481. Without it arrays don't work.

this PR does not contain any commits, closing.


improve Load More button UI

hey @1e9abhi1e10 I want to shift load more button on right side and want to improve button UI

Please assign this issue to me under GSSoC'23

Go ahead @devWonderkind


[pilot] fix default value of demo_account_required to nil instead of false

Checklist

[x] I've run bundle exec rspec from the root directory to see all new and existing tests pass
[x] I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
[x] I've read the Contribution Guidelines
[x] I've updated the documentation if necessary.

Motivation and Context
As discussed in #17910 pilot seems to be unable to work with an api_key with just the Developer permissions, needing App Manager permissions instead. However, it should work with the Developer permissions if no distribution of the uploaded binary is performed. This is the first in a series of patches.
Description
The demo_account_required has a default value of false but then on the code that chacks for its presence https://github.com/fastlane/fastlane/blob/832e3e4a19d9cff5d5a14a61e9614b5659327427/pilot/lib/pilot/build_manager.rb#L217 it performs a null check that always evaluates to true (false is not nil) and that causes an API call that needs the said App Manager permissions.
With this patch, I propose that option be made optional (and thus nil if not set) so that only when an explicit value is set, the API call is performed, avoiding the need for App Manager permissions.
Testing Steps
Before: If the option was not set, the false default would also trigger an API call
Now: If the option is not set, the nil value will prevent an API call.

I'm wondering if this kind of cases should be handled as exceptions instead of silently pass. If this parameter is set to true or false but you have no permissions, what happens in the submission?

@googlebot I consent.

Thanks to you @joshdholtz, the idea is to keep working on small patches to allow uploading testflight versions with Developer api key permissions instead of App Manager ones. More to come :)

I'm wondering if this kind of cases should be handled as exceptions instead of silently pass. If this parameter is set to true or false but you have no permissions, what happens in the submission?

It will throw an exception if there are no permissions on the api key for that api operation, it's what was happening to me, hence the fix!


Changing file position is advised in http::file_body but no methods provided for it

http::file_body has a good and convenient way for downloading large files by opening them with file_mode = append_existing. In the documentation (file_base.hpp) It is also advised to change file position:
"The current file position shall be set to the end of
the file prior to each write."
But I can't see any methods for accomplish this using basic_file_body::value_type class. Moreover, I see that "File file_" is private (not protected) - so I cannot add this functionality by overriding the class. I have to completely rewrite the class to accomplish the desired behavior.
Please add the necessary method (or an additional file_mode) for conveniently downloading large files.

I stumbled upon this as well, I solved it by manually creating a file object. Something like this:
boost::beast::http::response_parser<boost::beast::http::file_body> parser;
boost::system::error_code ec;
boost::beast::file file;
file.open("/path/to/file", boost::beast::file_mode::append_existing, ec);

//Seek to the end of the file
auto size(file.size(ec));
if (size > 0) {
    file.seek(size, ec);
}

//Put the file into the body
parser.get().body().reset(std::move(file), ec);

//...continue with read/async_read

For now, is there any harm in adding an accessor for the File to basic_file_body?
File& basic_file_body<File>::value_type::file() { return file_; }
I am trying to customize the File itself - computing a hash during the download - and need to get the hash out of my File object. To do this I have to return out a shared_ptr to internal state that I can access after the reset() move, among other complications.
If safe, I can pull-request this change, it's simple.

For now, is there any harm in adding an accessor for the File to basic_file_body?
File& basic_file_body<File>::value_type::file() { return file_; }
I came upon a need for this as well, trying to customize the File type and needing the reference to it without resorting to passing along a shared_ptr to internal state.
If it's safe, I can pull-request this change, it's simple.

Yes I think we can add that safely


Is it possible to find out driveable area based on HD map in motion dataset?

dear authors, thank you very much for releasing the data!
I want to find out the driveable area using the vector map provided in the motion dataset? (e.g. to see if a xy coordinate is in the driveable area) How can I do that?
can you help me? Thank you so much!

We do not provide a direct way to compute driveable area.  You will need to develop an algorithm for this.  We do provide lane centers, lane boundaries, and road edges which also contain direction vectors that you can use for this purpose.  Please let me know if you have further questions.

Hi @scott-ettinger,
May I know what types of map features constitute the boundary of the drivable area? Does it include 'ROAD_EDGE_UNKNOWN', 'ROAD_EDGE_BOUNDARY', and 'ROAD_EDGE_MEDIAN'? And what about the road line types?

Thanks for the reply.

In relation to this, I am trying to create a polygon of the road based on the waymo map in order to evaluate drivable area estimation algorimths. I am having a hard time doing so because the road boundaries are out of order. Any way I could streamline this process?


add support for changing the model with --model/-m

Does exactly what it says on the tin. Does not support custom models yet. Here is a usage example:

$ ./chatgpt -m text-davinci-003 -q "Explain what the following shell command does: awk 'NF>1'"

This command uses the awk utility to print the lines that contain more than one field (column). This can be used, for example, to process a text file and select all lines with more than one word.
$ ./chatgpt -m text-curie-001 -q "Explain what the following shell command does: awk 'NF>1'"

This shell command uses awk to print the first line of every file in the current directory, excluding the file named NF. Because NF is a file name, the command prints nothing, which is what you would expect.
$ ./chatgpt -m text-babbage-001 -q "Explain what the following shell command does: awk 'NF>1'"

This shell command prints the string "Yes, $1" to the console.

Thanks @charlesdaniels, I've been meaning to do something like this, a couple of thoughts
What if we check that the model is in a list?
Instead, let the API return an error, and present that to the user.  This ought to support custom models too?
Example usage
We should add a section to the long help to show an example of using a different / custom model. Adding a link to the long help string for where to find valid model names would help with the previous point.
model command in interactive prompt
It would be nice if we could switch models in interactive mode too.
A "model" case just before this one https://github.com/verdverm/chatgpt/blob/main/main.go#L400 with similar logic should be all it takes

Yeah I was waffling on whether to explicitly check it against a list or just bubble up the API error. Considering the new GPT3.5 release I think that validates the idea we should just accept and arbitrary string as you suggest.
Adding an example to the help with a usage example is a good idea.
I didn't realize about the interactive prompt, but yeah that should be simple since the currently selected model is just a string. I imagine you could get some funky results changing models half way through an interactive session, but if the user wants to do that, I don't see any reason to stop them from doing so.
I have kept meaning to get back to you with an updated PR, but I've been busy this week. I'll make these changes as soon as I get a chance :).

I implemented those changes, no issues.
The GPT3 library you were using has been replaced by go-openai by it's own author, so I went ahead and updated to that. I did test if gpt-3.5-turbo would work, however unfortunately it looks like it uses the new "chat completion" system. That will need a little extra donkey work to support.

Cool, looks good.
One last thing, can you replace the code block in the readme with the latest help message from the cli?

and also the interactive first help message

Thanks for the contribution @charlesdaniels
I'll update the readme post merge

Oops yeah I meant to add those two nits but I forgot. Thanks.


Add interactive colorful waves animation.

Is your feature request related to a problem? Please describe.
No its not a problem.
Describe the solution you'd like
I would like to add interactive colorful waves animation.

Additional context
Please assign me this under gssoc21

@ridsuteri @991rajat Please assign .

sorry @Shivangi-ch but this animation has already been applied to a project by the name "color flicker" :( you can either look for it's improvements or any other awesome js project


Thoughts on updating this package to work with the latest GPflow?

Hi, I recently came across your papers and this package and the work is really cool.
Do you have any plans on updating this package to work with the latest gpflow?

No worries, thanks for letting me know - I had a little go at translating various lines to the latest gpflow however, I'm not an expert in it so might not get very far. If I do I will write back here.
Cheers!


CloudStack fails to migrate VM with volume when there are datadisks attatched

Description
This PR fixes issue #5407. In summary, volumes that should not be migrated are still being mapped in VirtualMachineManagerImpl.createStoragePoolMappingsForVolumes.
To give some context: method createStoragePoolMappingsForVolumes is used only when migrating a VM with its volume(s), therefore via the API call migrateVirtualMachineWithVolume.
In such cases, there are two options:
A. User does not provide volumes, just the VM ID, and Host ID
B. User provides VM ID, Host ID, and a volume map, e.g. migrateto[0].volume=71f43cd6-69b0-4d3b-9fbc-67f50963d60b&migrateto[0].pool=a382f181-3d2b-4413-b92d-b8931befa7e1&migrateto[1].volume=88de0173-55c0-4c1c-a269-83d0279eeedf&migrateto[1].pool=95d6e97c-6766-4d67-9a30-c449c15011d1.
However, regardless of case A or B, createStoragePoolMappingsForVolumes iterates all the VM's volumes and adds to be migrated. Therefore, this causes exceptions when migrating VMs with data disk attached whenever the data-disk (shared) should not be migrated.
From my point of view, it does not make sense to add volumes that the user has not mapped to be migrated unless the VM is migrated to (I) different cluster or (II) the volume scope is LOCAL. Both of these cases are already covered by the if statement previous to the removed line in this PR.
if (ScopeType.HOST.equals(currentPool.getScope()) || isStorageCrossClusterMigration(plan.getClusterId(), currentPool)) {
                createVolumeToStoragePoolMappingIfPossible(profile, plan, volumeToPoolObjectMap, volume, currentPool);
}

Types of changes

[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] New feature (non-breaking change which adds functionality)
[X] Bug fix (non-breaking change which fixes an issue)
[ ] Enhancement (improves an existing feature and functionality)
[ ] Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale

[ ] Major
[ ] Minor

Bug Severity

[ ] BLOCKER
[X] Critical
[ ] Major
[ ] Minor
[ ] Trivial

How Has This Been Tested?

Migrate VM with Root disk in local storage.
Tests have been done via the UI as well as the API. Also, it considered on both cases:
A. User does not provide volumes, just the VM ID, and HostID;
B. User provides VM ID, and HostID, and a volume map, e.g. migrateto[0].volume=71f43cd6-69b0-4d3b-9fbc-67f50963d60b&migrateto[0].pool=a382f181-3d2b-4413-b92d-b8931befa7e1&migrateto[1].volume=88de0173-55c0-4c1c-a269-83d0279eeedf&migrateto[1].pool=95d6e97c-6766-4d67-9a30-c449c15011d1.
In both cases, the volume map has been done accordingly with the expected behavior.
VMs that fail to be migrated, have been successfully migrated and only the correct volume was migrated; data disks remained attached to the VM, at their storage pool, and in "Ready" state.

This has been tested with KVM.
Pinging some of the experts in managed storages (@mike-tutkowski @slavkap) to check if there are any cases I am missing where such volume mapping is correct.
Also, I would appreciate feedback regarding different hypervisors, such as VMware and Xen (@rhtyd @shwstppr @nvazquez @DaanHoogland).

@blueorangutan package

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1145

Tested offline migration on VMware env and the volume for which mapping was not provided in API call got detached from the VM in vCenter.

Migrated a stopped with a ROOT and a DATA disk within the cluster.
Both volumes were on same primary store before migration - ps1
Migrated with migrateVirtualMachineWithVolume API by passing a mapping for ROOT volume for primary store ps1-1

Thanks for reviewing and testing @shwstppr @nvazquez!
I will check different approaches as this could impact VMware migrations.
When another approach is ready for review I will ping you.

@shwstppr @nvazquez PR is ready for review.

I've found some issues when having Local + NFS attached, with Local + RBD data disk all works fine.
I will check the workflow with the NFS tests feedback.

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1209

@blueorangutan package

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1230

I am running a couple more tests. However, considering that the 4.16 cut will be soon, I might reduce the scope to fix only RBD (datadisk) + Local (Root). And then block migrations with NFS (datadisk) + Local (Root).

@blueorangutan test

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Trillian test result (tid-2109)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36867 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2109-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

@GabrielBrascher I've tested VM migration with volumes (ROOT disk on local storage + DATA disk on shared storage [NFS]), to another host within the same cluster. Migration failed with/without volumes mapping. Did I missed anything? Let me know if I've to test with any specific steps (and configuration).
Without volumes mapping =>
(cloudcmk) > migrate virtualmachinewithvolume virtualmachineid=455dc09d-7b05-42ad-89fb-3b0bcb9dc01e hostid=bf2abca7-0b54-4bdd-a3dc-0056ec706e18
{
  "accountid": "4f9b2858-1d1d-11ec-81eb-1e0079000453",
  "cmd": "org.apache.cloudstack.api.command.admin.vm.MigrateVirtualMachineWithVolumeCmd",
  "completed": "2021-09-24T11:02:43+0000",
  "created": "2021-09-24T11:02:40+0000",
  "jobid": "21f63e7d-967e-443d-8cbc-72322fb977a7",
  "jobprocstatus": 0,
  "jobresult": {
    "errorcode": 530,
    "errortext": "Failed to migrated vm VM instance {id: \"3\", name: \"i-2-3-VM\", uuid: \"455dc09d-7b05-42ad-89fb-3b0bcb9dc01e\", type=\"User\"} along with its volumes. "
  },
  "jobresultcode": 530,
  "jobresulttype": "object",
  "jobstatus": 2,
  "userid": "4f9cfd7a-1d1d-11ec-81eb-1e0079000453"
}
🙈 Error: async API failed for job 21f63e7d-967e-443d-8cbc-72322fb977a7

With volumes mapping =>
(cloudcmk) > migrate virtualmachinewithvolume virtualmachineid=455dc09d-7b05-42ad-89fb-3b0bcb9dc01e hostid=bf2abca7-0b54-4bdd-a3dc-0056ec706e18 migrateto[0].volume=898ee4b0-445a-4091-a402-0d52546f08c1 migrateto[0].pool=b67d9027-afcb-4a73-b24c-b1dc06a8d539 migrateto[1].volume=0d9efebe-c7ed-4030-92f9-33d4dfa50c87 migrateto[1].pool=3f039d92-52c0-3b68-88c5-8b27ea8e1fd2
{
"accountid": "4f9b2858-1d1d-11ec-81eb-1e0079000453",
"cmd": "org.apache.cloudstack.api.command.admin.vm.MigrateVirtualMachineWithVolumeCmd",
"completed": "2021-09-24T11:12:41+0000",
"created": "2021-09-24T11:12:38+0000",
"jobid": "5badf4d5-c2f9-40e9-b859-c1c082dfc1d6",
"jobprocstatus": 0,
"jobresult": {
"errorcode": 530,
"errortext": "Failed to migrated vm VM instance {id: "3", name: "i-2-3-VM", uuid: "455dc09d-7b05-42ad-89fb-3b0bcb9dc01e", type="User"} along with its volumes. "
},
"jobresultcode": 530,
"jobresulttype": "object",
"jobstatus": 2,
"userid": "4f9cfd7a-1d1d-11ec-81eb-1e0079000453"
}
🙈 Error: async API failed for job 5badf4d5-c2f9-40e9-b859-c1c082dfc1d6

@GabrielBrascher I've tested VM migration with volumes (ROOT disk on local storage + DATA disk on shared storage [NFS]), to another host within the same cluster. Migration failed with/without volumes mapping. Did I missed anything? Let me know if I've to test with any specific steps (and configuration).
Without volumes mapping =>
(cloudcmk) > migrate virtualmachinewithvolume virtualmachineid=455dc09d-7b05-42ad-89fb-3b0bcb9dc01e hostid=bf2abca7-0b54-4bdd-a3dc-0056ec706e18
{
  "accountid": "4f9b2858-1d1d-11ec-81eb-1e0079000453",
  "cmd": "org.apache.cloudstack.api.command.admin.vm.MigrateVirtualMachineWithVolumeCmd",
  "completed": "2021-09-24T11:02:43+0000",
  "created": "2021-09-24T11:02:40+0000",
  "jobid": "21f63e7d-967e-443d-8cbc-72322fb977a7",
  "jobprocstatus": 0,
  "jobresult": {
    "errorcode": 530,
    "errortext": "Failed to migrated vm VM instance {id: \"3\", name: \"i-2-3-VM\", uuid: \"455dc09d-7b05-42ad-89fb-3b0bcb9dc01e\", type=\"User\"} along with its volumes. "
  },
  "jobresultcode": 530,
  "jobresulttype": "object",
  "jobstatus": 2,
  "userid": "4f9cfd7a-1d1d-11ec-81eb-1e0079000453"
}
🙈 Error: async API failed for job 21f63e7d-967e-443d-8cbc-72322fb977a7

With volumes mapping =>
(cloudcmk) > migrate virtualmachinewithvolume virtualmachineid=455dc09d-7b05-42ad-89fb-3b0bcb9dc01e hostid=bf2abca7-0b54-4bdd-a3dc-0056ec706e18 migrateto[0].volume=898ee4b0-445a-4091-a402-0d52546f08c1 migrateto[0].pool=b67d9027-afcb-4a73-b24c-b1dc06a8d539 migrateto[1].volume=0d9efebe-c7ed-4030-92f9-33d4dfa50c87 migrateto[1].pool=3f039d92-52c0-3b68-88c5-8b27ea8e1fd2
{
  "accountid": "4f9b2858-1d1d-11ec-81eb-1e0079000453",
  "cmd": "org.apache.cloudstack.api.command.admin.vm.MigrateVirtualMachineWithVolumeCmd",
  "completed": "2021-09-24T11:12:41+0000",
  "created": "2021-09-24T11:12:38+0000",
  "jobid": "5badf4d5-c2f9-40e9-b859-c1c082dfc1d6",
  "jobprocstatus": 0,
  "jobresult": {
    "errorcode": 530,
    "errortext": "Failed to migrated vm VM instance {id: \"3\", name: \"i-2-3-VM\", uuid: \"455dc09d-7b05-42ad-89fb-3b0bcb9dc01e\", type=\"User\"} along with its volumes. "
  },
  "jobresultcode": 530,
  "jobresulttype": "object",
  "jobstatus": 2,
  "userid": "4f9cfd7a-1d1d-11ec-81eb-1e0079000453"
}
🙈 Error: async API failed for job 5badf4d5-c2f9-40e9-b859-c1c082dfc1d6

Will re-test with qemu ev pkgs and update here.

Tested with qemu ev pkgs, still migration failed with/without volumes mapping for the VM with ROOT disk on local storage + DATA disk on shared storage. Able to migrate VM with ROOT disk on shared storage + DATA disk on shared storage.

@GabrielBrascher can you test with ev packages, use mbx with CentOS7 if that helps.

@sureshanaparti thanks for testing.
With Root Local + Datadisk (NFS) it is failing for me as well. However, it did not fail with just Local.
I decided to revert some changes and change the approach, in a few minutes I will update here and explain the reasons behind the new approach to tackle this issue.

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1404

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1403

@blueorangutan test

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Trillian test result (tid-2223)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 118834 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2223-kvm-centos7.zip
Smoke tests completed. 59 look OK, 30 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

ContextSuite context=TestNestedVirtualization>:setup
Error
0.00
test_nested_virtualization.py

ContextSuite context=TestDeployVmWithAffinityGroup>:setup
Error
0.00
test_affinity_groups_projects.py

ContextSuite context=TestISOUsage>:setup
Error
0.00
test_usage.py

test_01_template_usage
Error
18.76
test_usage.py

test_01_volume_usage
Error
186.40
test_usage.py

ContextSuite context=TestAnnotations>:setup
Error
0.00
test_annotations.py

ContextSuite context=TestResetVmOnReboot>:setup
Error
0.00
test_reset_vm_on_reboot.py

ContextSuite context=TestRemoteDiagnostics>:setup
Error
0.00
test_diagnostics.py

ContextSuite context=TestAsyncJob>:setup
Error
0.00
test_async_job.py

ContextSuite context=TestNetworkACL>:setup
Error
0.00
test_network_acl.py

ContextSuite context=TestRouterDHCPHosts>:setup
Error
0.00
test_router_dhcphosts.py

ContextSuite context=TestRouterDHCPOpts>:setup
Error
0.00
test_router_dhcphosts.py

test_list_clusters_metrics
Error
1512.13
test_metrics_api.py

test_list_vms_metrics
Error
0.16
test_metrics_api.py

ContextSuite context=TestRouterDns>:setup
Error
0.00
test_router_dns.py

test_delete_account
Error
1511.83
test_network.py

test_delete_network_while_vm_on_it
Error
1.13
test_network.py

test_deploy_vm_l2network
Error
1.13
test_network.py

test_l2network_restart
Error
2.19
test_network.py

ContextSuite context=TestPortForwarding>:setup
Error
3.35
test_network.py

ContextSuite context=TestPublicIP>:setup
Error
1.11
test_network.py

test_reboot_router
Failure
0.04
test_network.py

test_releaseIP
Error
0.43
test_network.py

ContextSuite context=TestRouterRules>:setup
Error
0.48
test_network.py

ContextSuite context=TestDeployVMFromISO>:setup
Error
0.00
test_deploy_vm_iso.py

ContextSuite context=TestDeployVirtioSCSIVM>:setup
Error
0.00
test_deploy_virtio_scsi_vm.py

ContextSuite context=TestAdapterTypeForNic>:setup
Error
0.00
test_nic_adapter_type.py

ContextSuite context=TestRAMCPUResourceAccounting>:setup
Error
0.00
test_resource_accounting.py

ContextSuite context=TestPrivateGwACL>:setup
Error
0.00
test_privategw_acl.py

ContextSuite context=TestDeployVmWithUserData>:setup
Error
0.00
test_deploy_vm_with_userdata.py

ContextSuite context=TestDeployVmWithVariedPlanners>:setup
Error
0.00
test_deploy_vms_with_varied_deploymentplanners.py

ContextSuite context=TestPortForwardingRules>:setup
Error
0.00
test_portforwardingrules.py

ContextSuite context=TestProjectSuspendActivate>:setup
Error
1518.63
test_projects.py

ContextSuite context=TestDomainsServiceOfferings>:setup
Error
1515.16
test_domain_service_offerings.py

ContextSuite context=TestRouterDnsService>:setup
Error
0.00
test_router_dnsservice.py

test_06_download_detached_volume
Error
300.11
test_volumes.py

ContextSuite context=TestVolumes>:teardown
Error
248.19
test_volumes.py

ContextSuite context=TestInternalLb>:setup
Error
0.00
test_internal_lb.py

test_01_create_iso_with_checksum_sha1
Error
66.53
test_iso.py

test_02_create_iso_with_checksum_sha256
Error
66.46
test_iso.py

test_03_create_iso_with_checksum_md5
Error
66.39
test_iso.py

test_04_create_iso_with_no_checksum
Error
66.44
test_iso.py

test_01_create_iso
Failure
1512.22
test_iso.py

ContextSuite context=TestISO>:setup
Error
3023.58
test_iso.py

ContextSuite context=TestSnapshotRootDisk>:setup
Error
0.00
test_snapshots.py

test_01_invalid_upgrade_kubernetes_cluster
Failure
0.01
test_kubernetes_clusters.py

test_02_deploy_and_upgrade_kubernetes_cluster
Failure
0.01
test_kubernetes_clusters.py

test_03_deploy_and_scale_kubernetes_cluster
Failure
0.01
test_kubernetes_clusters.py

test_04_basic_lifecycle_kubernetes_cluster
Failure
0.01
test_kubernetes_clusters.py

test_05_delete_kubernetes_cluster
Failure
0.01
test_kubernetes_clusters.py

test_06_deploy_invalid_kubernetes_ha_cluster
Failure
0.01
test_kubernetes_clusters.py

test_07_deploy_kubernetes_ha_cluster
Failure
0.01
test_kubernetes_clusters.py

test_08_deploy_and_upgrade_kubernetes_ha_cluster
Failure
0.01
test_kubernetes_clusters.py

test_09_delete_kubernetes_ha_cluster
Failure
0.01
test_kubernetes_clusters.py

test_nic_secondaryip_add_remove
Error
1512.58
test_multipleips_per_nic.py

test_01_add_delete_kubernetes_supported_version
Error
1802.58
test_kubernetes_supported_versions.py

@rhtyd @nvazquez @shwstppr @GutoVeronezi thanks for the review. I am adding a few contexts and conclusions on how this implementation has evolved. I did a few changes on past Friday and have tested it since then [see this comment].
Recap and adding context on this PR
There were two options to fix:

Before calling the copy command -- map only the volumes that are going to be migrated which are either just the root disk, or root disk (Local) + data disks (shared) that the user has explicitly added to be migrated in the API call.
At the KvmNonManagedStorageDataMotionStrategy -- this change would impact on the "canHandle" and add a few changes in the workflow in order to "ignore" some of the mapped volumes; which require more work to ensure the volumes should indeed be ignored.

Considerations for each option
Option 1: it is interesting as so far works like a charm and it is simple to implement; however, it requires specific logic for KVM in the "generic" class for migrating VM.
Option 2: makes sense to narrow it to the KVM  strategy; however, at that point, all VMs are mapped as migrating in the DB and it does not make much sense to have such volumes in the migrating state. The implementation also was a lot more complex to be done.
Tests and conclusion
I was successful with the Local + RBD migrations. looks like a great addition to 4.16 in this scenario.
However, Local + NFS still not working on my tests.

@blueorangutan test

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@GabrielBrascher I've tested VM migration with volumes. Results below.

VM with Local ROOT disk

(i) Without volumes mapping:
migrate virtualmachinewithvolume virtualmachineid=4c5cf633-482d-499f-8d39-970fa1f62efc hostid=d19ac1e9-990d-404d-b60c-a7b75cf7836c
=> Failed with error: "Failed to migrated vm VM instance {id: "7", name: "i-2-7-VM", uuid: "4c5cf633-482d-499f-8d39-970fa1f62efc", type="User"} along with its volumes. "
(ii) With volumes mapping (Local storage on src host : Local storage on dest host):
migrate virtualmachinewithvolume virtualmachineid=4c5cf633-482d-499f-8d39-970fa1f62efc hostid=d19ac1e9-990d-404d-b60c-a7b75cf7836c migrateto[0].volume=703abf09-99f7-48b8-9a47-55a467452a60 migrateto[0].pool=cae2c100-cfd8-47f1-aed5-3442b289ec85
=> Failed with error: "Failed to migrated vm VM instance {id: "7", name: "i-2-7-VM", uuid: "4c5cf633-482d-499f-8d39-970fa1f62efc", type="User"} along with its volumes. "

VM with Local ROOT disk + Shared (NFS) DATA disk

(i) Without volumes mapping:
migrate virtualmachinewithvolume virtualmachineid=3a879829-466a-451a-bb2d-a2897349a9d9 hostid=d19ac1e9-990d-404d-b60c-a7b75cf7836
=> Passed
(ii) With ROOT disk volume mapping (Local storage on src host : Local storage on dest host):
migrate virtualmachinewithvolume virtualmachineid=3a879829-466a-451a-bb2d-a2897349a9d9 hostid=d19ac1e9-990d-404d-b60c-a7b75cf7836c  migrateto[0].volume=c01302c0-41e4-4fbe-876e-13c3493ad8f6 migrateto[0].pool=cae2c100-cfd8-47f1-aed5-3442b289ec85
=> Passed
(iii) With ROOT & DATA disk volume mapping (Local storage on src host : Local storage on dest host + NFS storage1 : NFS storage2):
migrate virtualmachinewithvolume virtualmachineid=3a879829-466a-451a-bb2d-a2897349a9d9 hostid=d19ac1e9-990d-404d-b60c-a7b75cf7836c  migrateto[0].volume=c01302c0-41e4-4fbe-876e-13c3493ad8f6 migrateto[0].pool=cae2c100-cfd8-47f1-aed5-3442b289ec85 migrateto[1].volume=9f2f170a-6650-4750-bd9c-76fca2ae70cb migrateto[1].pool=df3aba77-9ff6-3c91-9062-f2d79686672e
=> Failed with error: "Failed to migrated vm VM instance {id: "5", name: "i-2-5-VM", uuid: "3a879829-466a-451a-bb2d-a2897349a9d9", type="User"} along with its volumes. "

VM with Shared (NFS) ROOT disk

(i) With ROOT disk volume mapping:
migrate virtualmachinewithvolume virtualmachineid=b56072e6-88d1-4c13-a6e7-b9ab76e54b82 migrateto[0].volume=bde7a8ee-85ed-4ed5-bfd4-4c2de38fba04 migrateto[0].pool=df3aba77-9ff6-3c91-9062-f2d79686672e hostid=999460c3-07e7-4eae-867a-c3747ce36589
=> Failed with error: "Failed to migrated vm VM instance {id: "3", name: "i-2-3-VM", uuid: "b56072e6-88d1-4c13-a6e7-b9ab76e54b82", type="User"} along with its volumes. "

VM with Shared (NFS) ROOT disk + Shared (NFS) DATA disk

(i) With ROOT & DATA volumes mapping:
migrate virtualmachinewithvolume virtualmachineid=2d94bc0e-9de1-4d0f-ae67-bf69286a2fdf hostid=999460c3-07e7-4eae-867a-c3747ce36589 migrateto[0].volume=6f99fb94-b4d9-462f-8e26-227f3d33f946 migrateto[0].pool=df3aba77-9ff6-3c91-9062-f2d79686672e migrateto[1].volume=2b1ec21e-d02a-441d-ab3e-ca831eb4c6a6 migrateto[1].pool=df3aba77-9ff6-3c91-9062-f2d79686672e
=> Failed with error: "Failed to migrated vm VM instance {id: "6", name: "i-2-6-VM", uuid: "2d94bc0e-9de1-4d0f-ae67-bf69286a2fdf", type="User"} along with its volumes. "

@GabrielBrascher I noticed the most of the migration with volumes fails, not sure if these are regression. Let me know if I have to check any specific cases.

Trillian test result (tid-2254)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33118 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2254-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

@GabrielBrascher @sureshanaparti is this PR ready?

@GabrielBrascher @sureshanaparti is this PR ready?

Not yet @nvazquez , Observed some failures in manual tests, waiting for the confirmation from @GabrielBrascher.

@sureshanaparti sorry for the delay, I have been debugging and running some tests:
I am also pushing a commit (tested recently on it as well) which I am addressing a review from @GutoVeronezi.
These are the output of my recent tests:
VM With local Root disk
(i) Without volumes mapping:
migrate virtualmachinewithvolume virtualmachineid=1f83b074-2d25-4eb1-9e10-bf26ccb71e50 hostid=a19dcabd-53b6-44e4-9f57-2fa70d876ef8
=> Success
(ii) With ROOT volume mapping:
migrate virtualmachinewithvolume virtualmachineid=1f83b074-2d25-4eb1-9e10-bf26ccb71e50 hostid=448a0207-975b-41e1-af07-0c6e55dae0b3 migrateto[0].volume=e6e28756-0beb-4f75-8e6e-9ba5cc50b983 migrateto[0].pool=887fe175-7422-4e3a-a9ed-392932afa631
=> Success
VM With local Root disk + Shared (RBD) DATA disk
(i) Without volumes mapping:
migrate virtualmachinewithvolume virtualmachineid=1f83b074-2d25-4eb1-9e10-bf26ccb71e50 hostid=448a0207-975b-41e1-af07-0c6e55dae0b
=> Success
(ii) With ROOT volume mapping:
migrate virtualmachinewithvolume virtualmachineid=0f6c2e2d-7960-4715-b0c9-8634ccc36548 hostid= migrateto[0].volume=e6e28756-0beb-4f75-8e6e-9ba5cc50b983 migrateto[0].pool=2bfa2e18-d76f-46db-8605-4aafb1438030
=> Success
VM With local Root disk + Shared (NFS) DATA disk
(i) Without volumes mapping:
migrate virtualmachinewithvolume virtualmachineid=1f83b074-2d25-4eb1-9e10-bf26ccb71e50 hostid=448a0207-975b-41e1-af07-0c6e55dae0b
=> Failed with error:"Copy volume(s) to storage(s) [{volume: "13433", from: "23", to:"26"}] and VM to host [{vm: "13452", from: "24", to:"46"}] failed in StorageSystemDataMotionStrategy.copyAsync. Error message: [Exception during migrate: org.libvirt.LibvirtException: internal error: unable to execute QEMU command 'nbd-server-add': Block node is read-only]."
In conclusion, all tests worked fine for me, except by the one with NFS datadisk (which seem to work with you).
I assume that the exception might be due to configurations in the NFS pool or the Libvirt, due to "LibvirtException: internal error: unable to execute QEMU command 'nbd-server-add': Block node is read-only."

@blueorangutan package

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: :heavy_multiplication_x: el7 :heavy_check_mark: el8 :heavy_multiplication_x: debian :heavy_check_mark: suse15. SL-JID 1503

@blueorangutan package

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1505

@blueorangutan test keepEnv

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Trillian test result (tid-2317)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35741 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2317-kvm-centos7.zip
Smoke tests completed. 86 look OK, 5 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

test_01_add_primary_storage_disabled_host
Error
0.57
test_primary_storage.py

test_01_primary_storage_nfs
Error
0.10
test_primary_storage.py

ContextSuite context=TestStorageTags>:setup
Error
0.18
test_primary_storage.py

test_01_secure_vm_migration
Error
165.58
test_vm_life_cycle.py

test_02_unsecure_vm_migration
Error
276.61
test_vm_life_cycle.py

test_03_secured_to_nonsecured_vm_migration
Error
149.26
test_vm_life_cycle.py

test_08_migrate_vm
Error
44.82
test_vm_life_cycle.py

test_03_deploy_and_scale_kubernetes_cluster
Failure
33.91
test_kubernetes_clusters.py

test_07_deploy_kubernetes_ha_cluster
Failure
44.11
test_kubernetes_clusters.py

test_08_deploy_and_upgrade_kubernetes_ha_cluster
Failure
61.59
test_kubernetes_clusters.py

test_09_delete_kubernetes_ha_cluster
Failure
51.36
test_kubernetes_clusters.py

ContextSuite context=TestKubernetesCluster>:teardown
Error
96.14
test_kubernetes_clusters.py

test_02_list_snapshots_with_removed_data_store
Error
9.48
test_snapshots.py

test_02_list_snapshots_with_removed_data_store
Error
9.49
test_snapshots.py

test_hostha_enable_ha_when_host_in_maintenance
Error
303.79
test_hostha_kvm.py

@blueorangutan test matrix

Trillian test result (tid-2339)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33255 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2339-xenserver-71.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

Trillian test result (tid-2341)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37299 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2341-vmware-65u2.zip
Smoke tests completed. 90 look OK, 1 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

test_create_pvlan_network
Error
0.03
test_pvlan.py

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

Trillian test result (tid-2344)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32020 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2344-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

Trillian test result (tid-2349)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36103 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2349-vmware-67u3.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test
Result
Time (s)
Test File

@sureshanaparti @vladimirpetrov @borisstoyanov @nvazquez do we need any further manual testing before merging this?

@rhtyd yes, requires some manual tests for VMware migration cases.


[WIP] Atomic API

This is an unfinished proposal for a better Atomic API.
Current API in Crossbeam
Current API in coco
I may be biased, but the one in coco feels simpler and more ergonomic. Naturally, I'm leaning in this direction. This statement is based on my experience of implementing a lock-free skiplist using both APIs. I'd love to show some evidence for that, but don't have any at the moment. Sorry!
Rendered
This RFC is still unfinished. Help is needed.
@jeehoonkang I'd especially love some help from you with this one :)

I'm still thinking about this but:
2.1 - Provide an unsafe variant that takes memory ordering (cas_weak, ...) and a safe one that's always seqcst (cas_weak_seq_cst, ...).

I'm a bit hesitant on this but I thought I'd share anyway.
We could use the type system to provide type-safe Nullability and Taggabiliy, something along the lines of:
trait Nullability;
trait Taggabiliy;
these traits will provide some methods that will be implemented by
struct Nullable;
struct NonNullable;
struct Taggeable;
struct NonTaggeable;
And ultimately be used by the structs Ref, Atomic and their methods.
struct Ref<'a, T, X: Nullability, Z: Taggabiliy = NonTaggeable> {.. }
struct Atomic<T, Z: Taggabiliy = NonTaggeable> { .. }
This is not a full description but I'm hopping to start a conversation at least.
I've seem the type system being used for similar things with good success, like:

Tendril used for Encoding (even type safe conversions between them) and Atomicity.
HashMap bucket this one is a bit too extreme as it one goes two steps further and propagate mutability and can be recursive (by abusing trait Deref), but it ends up providing a safe-ish interface with almost zero overhead.

As discussed in https://github.com/stjepang/coco/pull/2, I think:

crossbeam-epoch and epoch-based memory reclamation scheme in general resolve the races between read/writes and deallocations of a location, but not the races among read/writes.  That is the burden of the user of crossbeam-epoch.
The Rust type system cannot express the idea of race-freedom, and thus the read/write operations (at least some of them) should be marked as unsafe.
@arthurprs 's proposal (providing two methods: one w/ ordering argument and the other w/ seq_cst) is tempting.  In fact, acq_rel should be enough for safe API.  But in my experience, almost all the concurrent data structures implemented in C/C++11 anyway requires relaxed read/writes (or just release or acquire updates), and they all end up with using unsafe interface.  Then I think we can get only a little by this dual-method API design.  Anyway we need to investigate all the accesses w.r.t. race-freedom.
So I propose to mark all the read/write operations as unsafe.

I've not had time to digest these APIs in detail, however I did want to get some clarification about one assertion made in the RFC (I'm not that familiar with crossbeam's API, so I'm looking into it as I go):

By using relaxed orderings we can create a data race within safe code. This is unsound, as Rust must not allow data races in safe code.

The data-race here is specifically because the load API offers an Option<Shared<T>>, right? That is, because Shared<T> implements Deref, we are basically asserting that if you get your hands on one, it is safe to deref -- but with the Relaxed store, there is no reason for that to be true. (In contrast, the AtomicPtr API in the standard library gives yo back a *mut T, meaning you have to decide for yourself if it is safe to deref.)

@nikomatsakis Exactly. Dereferencing is the crucial operation here.
We can say that dereferencing is simply unsafe and call it a day. Or, as @jeehoonkang suggests, we can simply mark all load/store/swap/cas/cas_weak methods as unsafe. But that sounds like capitulation to me. :)
I believe we can design an API that is both safe and performant for 95% use cases, and would like to push for that. This is Rust's overall philosophy anyway: let's make most of the code easy and safe, and apply unsafe blocks in the few special cases.

2.1 - Provide an unsafe variant that takes memory ordering (cas_weak, ...) and a safe one that's always seqcst (cas_weak_seq_cst, ...).

But Acquire/Release/AcqRel are safe as well. Generally speaking, I think memory orderings for all atomic operations fall into three categories:

Relaxed ordering. This is unsafe.
Acquire (for load), Release (for store), AcqRel (for swap, cas, and cas_weak). This is safe.
SeqCst ordering (just like 2nd category, but with guaranteed global total order). This is also safe.

When writing concurrent data structures, I believe that the 2nd and 3rd are equally commonly needed, while the 1st is pretty rare.
For example, to allow full flexibility for store operation, we could have the following methods:

store_release
store_seq_cst
store_box_release
store_box_seq_cst
store_raw (unsafe)

Unsafe methods store_relaxed and store_box_relaxed are not very compelling. Since they're already unsafe (and I believe rare), you can just as well use store_raw.

We could use the type system to provide type-safe nullability and taggabiliy for zero-cost safety, something along the lines of:

This is certainly an interesting approach, although I haven't come up with a satisfying solution in this direction yet.

But Acquire/Release/AcqRel are safe as well.

Sure, but the idea is to limit the number methods.
I'm not confident it's possible o provide an actual safe interface. It's gonna be really tricky to write a concurrent data structure without unsafe somewhere anyway.

Another possibility is to let methods accept an argument that tells whether they are sequentially consistent or not. For example:
// AcqRel when false, SeqCst when true
fn swap<'a>(&self, new: Ptr<'a, T>, seq_cst: bool) -> Ptr<'a, T>;

// Just to avoid confusion: `Ptr` holds a (possibly null) pointer and a tag encoded within its unused bits.

Then, with swap_raw you have full control over precise ordering when you need it:
fn swap_raw<'a>(&self, new: *mut T, tag: usize, order: Ordering) -> Ptr<'a, T>;

Perhaps even macros could help to allow syntax like:
cas!(head, old, new); // SeqCst by default
cas!(head, old, new, AcqRel);
cas!(head, old, Box::new(Node::new()), SeqCst);

@stjepang oops!  I made a mistake in writing the previous comment.  I meant to mark dereferences as unsafe.  Sorry for confusion.

I believe we can design an API that is both safe and performant for 95% use cases, and would like to push for that. This is Rust's overall philosophy anyway: let's make most of the code easy and safe, and apply unsafe blocks in the few special cases.

I don't agree with this characterization.  In Crossbeam master, Chase-Lev dequeue (https://github.com/crossbeam-rs/crossbeam/blob/master/src/sync/chase_lev.rs#L195), MS queue (https://github.com/crossbeam-rs/crossbeam/blob/master/src/sync/ms_queue.rs#L89), segmented queue (https://github.com/crossbeam-rs/crossbeam/blob/master/src/sync/seg_queue.rs#L63), and Treiber stack (https://github.com/crossbeam-rs/crossbeam/blob/master/src/sync/treiber_stack.rs#L35) all have at least one relaxed access.  In general, in my experience in studying non-blocking data structures in C/C++11 memory models, almost every data structure is using relaxed accesses.
Also, I think crossbeam-epoch is essentially unsafe: it is well-known that non-blocking data structures are really hard to implement, and the difficulty is beyond what Rust's type system can give to us.  Yes, I am proposing to "give up", but I think the burden of ensuring race-freedom (except for the races with deallocation) in the data structures is at the user's hand, not ours.

I spend some time with the type system to check if (based on https://github.com/crossbeam-rs/rfcs/pull/2#issuecomment-298902593) could work and it appears so https://gist.github.com/arthurprs/3da8f45f99c6acb663cc09c49888147d

@stjepang what about a builder-like pattern. Give acquire-release by default, but allow opt-in to other orderings:
ptr.seqcst().swap(...);
unsafe { ptr.relaxed().swap(...); }

I have a comment on the tagged pointers.  This may be relevant in a far future, but...
Suppose the consume ordering will be introduced in Rust in the future (possibly very far from now).  Then the tagged pointers cannot be used for the consume ordering, since they are implemented with AtomicUSize.  Note that the dependency for the consume ordering is carried only to the pointer types, but not integer types such as AtomicUSize.
This may not be an issue for Rust or Crossbeam, since consume is very premature and the conflict between consume and pointer tagging is recognized by the C/C++ standard committee and the people are discussing how solve it.  I am just writing down this comment for noting that consume may be relevant to the design choices we are making here..

@jeehoonkang Are you sure you aren't mixed up consume ordering? Consume ordering can depend on non-pointer loads, take this code:
int mt_values[];
std::atomic<int> mt_index;
{
      int which_index = mt_index.load(std::memory_order_consume);
      int value = mt_values[mt_index]; // address dependency on which_index
}

There are issues like this where the compiler optimizes the dependency away, but I can't find any issues about tagged pointers, working only with integers, or anything in the standard which indicates consume only applies to pointer types. The standard is worded in such a way to imply that any value type loaded with consume can order loads in a dependency chain.

@schets Sorry, I should have cited the document I just implied: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0190r3.pdf http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0462r1.pdf  The standard committee agrees that the current definition of consume is unusable, and is preparing a fix to that.  The document says:
Another conspicuous change is that this proposal does not require dependencies to be carried by integers, but instead only by pointers.  Note that versions v4.2  and  later  of  the  Linux  kernel  avoid  carrying  dependencies  through  integer  variables  [15].   At  first  glance,  it  might  seem  that  restricted  carrying  of dependencies through intptr_t and uintptr_t is needed to tag pointers, but such tagging is not supported by the current C++ standard.  This document therefore completely prohibits carrying dependencies through integers.

That isn't part of the standard nor is it going to be in c++17, so I don't think we should worry about it. I find it absurd that they would prevent bit manipulation of pointers from being carried as a dependency, and expect the proposal will either allow for such behavior or a standardization proposal for that will be submitted later.
It might be worth thinking about if the pointer tagging scheme would cause the compiler to do anything funky, although LLVM and any other respectable c++ compiler could never do that or else they would break every single program out there. If this is ever expected to support architectures that don't use standard pointers (near/far scheme, 32 bit chars, something exotic?) we could think about that as well.

@schets yeah, the standard committee recognizes pointer tagging is essential, and surely they will submit another memorandum to deal with it.  I am leaving a comment just to note that there is a discussion on it.  I agree with you that the standard thing is not an important issue for now, and it is more urgent to see if the existing compilers will work fine.

Getting rid of the tag argument in functions is a great change, I can get behind this interface.
It seems to me that optional pointer tagging is a good use case for type parameter like I showed up-thread. Although in that example I included nulling as well, which I think may not carry it's weight.

I've updated the RFC. This is now a real proposal and not a draft anymore. :)
@arthurprs Your idea of providing optional nullability and taggability is interesting. However, I think the right choice here is: Always support nullability and taggability. Having other combinations available is just not worth the hassle, IMO.
@nikomatsakis Builder-like pattern at first looks like a potential solution, but feels a tad too odd, since it's inconsistent with existing conventions (passing ordering as an argument). But more importantly...
@jeehoonkang I think it's best to do what you proposed long time ago: just mark deferencing as unsafe and call it a day. An API that allows safe dereferencing is too difficult to design and probably still too impractical in the end. So I gave up trying to pursue complete safety. :)

Having other combinations available is just not worth the hassle, IMO.

It don't need to be a hassle. From what I learned in that example, tagging as a type parameter in the api you proposed now is really simple and will make it zero cost regarding tags.

Hey, thanks for the RFC! It's great to see crossbeam moving forward to better foundations!

Consider a node in a skiplist. It consists of: key, value, tower. The tower is an array of atomic pointers. To save a level of indirection, it is wise to lay out the entire tower inside the node. This means that nodes are dynamically sized.

A somewhat space-wasting solution is to have a tower array of constant size MAX_LEVELS with some levels empty, or None. It's not too bad, since afaik you can prove that at most (lg n) levels are necessary with high probability.

Yes, I am proposing to "give up", but I think the burden of ensuring race-freedom (except for the races with deallocation) in the data structures is at the user's hand, not ours.

Why this defeatism? :D It generally happens in Rust that internals of high-performance libraries would be unsafe, and so is the case for crossbeam-epoch, which isn't really meant to be widely used anyway. But this does not prevent us from providing safe and easy-to-use structures built on top of epoch, which is really the end goal.
There's a bit of an issue with tagged pointers. To be taggable, a pointer has to have alignment of at least 2. How would the API behave when a pointer only has alignment 1? Specifically, what does tag(&self) return in such a case? I see two options. One is to make it return Option<usize>, which would be slightly burdensome. The other is to just agree that an untaggable pointer always has tag 0 and mention that in the documentation.

Additionally, I believe it would be worth it to provide both compare-and-set and compare-and-swap, changing the naming of the appropriate methods in accordance with crossbeam-rs/crossbeam#116 to eliminate confusion about what cas stands for.
Another related issue is crossbeam-rs/crossbeam#113 and the proposal seems to solve it :)
I noticed something more worrying. Since there is no store_owned, storing a new value that we just constructed from e.g. a Box requires using Owned::into_shared, and that requires a pin, even though it's unnecessary, since we're not getting a Shared back! This incurs non-negligible overhead, so I propose adding store_owned back.

Thank you for finishing up the RFC.  IMO this is a huge achievement!  I just have two comments.

I agree with @Vtec234 in the naming of cas.  I think it is convenient to follow the API of Rust std: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicPtr.html.  I made an PR on this (https://github.com/crossbeam-rs/crossbeam/pull/128/files).

"By using relaxed orderings we can create a data race within safe code. This is unsound, as Rust must not allow data races in safe code.": In the world of C/C++, data race on relaxed accesses is safe.  Data race on nonatomic accesses is unsafe.  It is possible that Rust will just reuse the C/C++ concurrency memory model (which is not clearly decided yet; see more details in https://github.com/nikomatsakis/rust-memory-model).  Here the problem is, by using relaxed ordering you may read uninitialized memory, which is clearly a severe problem.  More concretely, to quote https://github.com/crossbeam-rs/crossbeam/issues/137, "The dereference of a Shared<T>, may race with the writer of the shared data of type T.  For example, suppose a Box<T> is written to Atomic<T> with store_ref(Relaxed), and later another thread read the Atomic<T> with load(Relaxed).  Then the Box<T> itself may not have been synchronized, and the dereference of the loaded Shared<T> may invoke undefined behavior."

@stjepang This is close to what I had in mind, https://gist.github.com/arthurprs/3da8f45f99c6acb663cc09c49888147d
The important bit is
pub struct Atomic<T, Z: Taggabiliy=NonTaggeable> {
}

pub struct Shared<'p, T: 'p, Z: Taggabiliy=NonTaggeable> {
}

pub mod tagged {
    pub type Atomic<T> = super::Atomic<T, super::Taggeable>;
    pub type Shared<'a, T> =  super::Shared<'a, T,  super::Taggeable>;
}

Which makes tagging free for modules that don't need them use crossbeam::atomic::{Atomic, ..}
and users opt-in by importing stuff from tagged submodule use crossbeam::atomic::tagged::{Atomic, ...}
So no hassle, no duplicated code and don't pay for you don't use.

I'll write a sample how it could look like.

That'd be helpful! So, the question is "is it worth splitting taggable and non-taggable atomics at all?" I think not, since the cost of ptr & MASK is really negligible in both code size and runtime cost. :) The other issue is that tagged atomics force proper alignment, but is that a real concern?
By the way, after moving MsQueue and TreiberStack to taggable atomics I didn't notice any difference in performance.

A somewhat space-wasting solution is to have a tower array of constant size MAX_LEVELS with some levels empty, or None. It's not too bad, since afaik you can prove that at most (lg n) levels are necessary with high probability.

The average tower height is 1 + 1/2 + 1/4 + 1/8 + ... = 2. So, if all nodes have a tower of height 32, while the average is 2, that's a lot of wasted space. :) That also contributes to worse cache locality, therefore making the perfomance substantially worse (I did the benchmarking).

There's a bit of an issue with tagged pointers. To be taggable, a pointer has to have alignment of at least 2. How would the API behave when a pointer only has alignment 1? Specifically, what does tag(&self) return in such a case?

Well, if a pointer has alignment of 1, the only value a tag can have is zero. The maximum value a tag can have is alignment - 1. Method with_tag should panic if the tag is too large. We could return an optional value to indicate whether tagging succeeded, but would anyone ever use that? Everyone would just call .unwrap() right after tagging. :)

Additionally, I believe it would be worth it to provide both compare-and-set and compare-and-swap, changing the naming of the appropriate methods in accordance with crossbeam-rs/crossbeam#116 to eliminate confusion about what cas stands for.

The difference between those two is that compare-and-swap is strictly stronger (it performs a load on failure) and requires a pin/guard. Is there a real scenario where compare-and-set is more desirable? Yes, compare-and-set is more desirable when the thread is not pinned, but why would someone actually want to do CAS on an Atomic when it isn't pinned? A concrete example here would be illuminating.
Compare-and-swap is a very common operation, so I'd strongly prefer having the name short. Especially because it takes four arguments. Arguably, the weak variant is more commonly used than the strong variant, so if we go with longer names, this might be the most common use of CAS in practice:
match self.head.compare_and_swap_weak_owned(head, node, AcqRel, pin) {
    Ok(_) => break,
    Err((h, n)) => {
        head = h;
        node = n;
    }
}

Alternatives to compare_and_set and compare_and_swap might be cmp_set and cmp_swap, or maybe cas and cmpxchg. The proposed cas is more similar to compare_exchange rather than compare_and_swap in libstd anyway. BTW, I find it silly that compare-and-set is not named compare-and-store instead. :)
To summarize, I personally don't think we need a split between compare-and-set and compare-and-swap. But if you do, I'd like to ask for the following:

A not too verbose naming scheme.
A convincing case demonstrating the need for compare-and-set.

I noticed something more worrying. Since there is no store_owned, storing a new value that we just constructed from e.g. a Box requires using Owned::into_shared, and that requires a pin, even though it's unnecessary, since we're not getting a Shared back! This incurs non-negligible overhead, so I propose adding store_owned back.

This is another method for which I haven't yet found a compelling use case. If you do have any, let me know. :) But I'm not as opposed to this idea - adding store_owned back sounds reasonable to me.

In the world of C/C++, data race on relaxed accesses is safe. Data race on nonatomic accesses is unsafe.

Oh, I guess my statement on data races was worded poorly. Should've been more clear on that accessing unsynchronized non-atomic data is the real problem. I'll try fixing it.

I personally prefer the longer names and compare-and-set, solely because it follows the naming scheme of libstd.  What do you think of this aspect?

@arthurprs Thanks for writing this!
It's a really neat trick. I just tried running cargo doc --open on that piece of code, but the added complexity stands out a bit. We can fix some rough edges with type aliases, but some will remain, e.g. signatures like:
fn load<'p>(&self, ord: Ordering, _: &'p Pin) -> Shared<'p, T, Z>
fn store<'p>(&self, ord: Ordering, new: Shared<'p, T, Z>)

This is not too complex nor too much of a hassle, but still something that adds complexity. Let me try to summarize my stance. It's all a matter of tradeoffs.
Advantages:

Users don't need to pay the price (one bitwise AND with a constant) of masking the pointer.
Users can have pointers to misalinged data.

Disadvantages:

Added layer of complexity. The docs don't look as clean.

Perhaps you'll disagree, but for me the disadvantage is greater than the sum of advantages. :) It'd be good to hear what others think as well. @jeehoonkang, @Vtec234 ?
@jeehoonkang

I personally prefer the longer names and compare-and-set, solely because it follows the naming scheme of libstd. What do you think of this aspect?

Well, cas and cas_owned do resemble compare_and_set and compare_exchange, but the differences are subtle:
// Returns current on success, or loads on failure.
fn compare_and_swap(
    &self,
    current: *mut T,
    new: *mut T,
    order: Ordering
) -> *mut T;

// Returns current on success, or loads on failure.
fn compare_exchange(
    &self,
    current: *mut T,
    new: *mut T,
    success: Ordering,
    failure: Ordering
) -> Result<*mut T, *mut T>;

// Returns () on success, or loads on failure.
fn cas<'p>(
    &self,
    current: Shared<T>,
    new: Shared<T>,
    ord: Ordering,
    _: &'p Pin,
) -> Result<(), Shared<'p, T>>;

// Returns new on success, or loads on failure.
fn cas_owned<'p>(
    &self,
    current: Shared<T>,
    new: Owned<T>,
    ord: Ordering,
    _: &'p Pin,
) -> Result<Shared<'p, T>, (Shared<'p, T>, Owned<T>)>;

I'd generally lean towards established conventions, but in this case we have small differences so conflating methods in Crossbeam with those in libstd under the same names is not as appealing. Also, data structures in Crossbeam will heavily use CAS operations, and things like self.head.compare_and_swap_weak_owned(head, node, AcqRel, pin) will be common and look terrible. :)
As a side note, the silent voice in my head doesn't say "compare-and-swap this pointer with that one" but rather "install this pointer if the old is that one". Or just "cas this pointer". It's a common method so I like it terse.
Another issue is that I believe we don't need both compare-and-swap and compare-and-set (explanation at the end of this comment), so being explicit about what particular flavor of CAS is in question is not as important.
@Vtec234 Just to elaborate a bit on your comment regarding store_owned...
I feel okay about the current API, but not great. Some odd aspects stand out, e.g. you can store a value without a pin, but you can't CAS without a pin. Also, load_raw feels out of place.
On a more higher level, I think Atomic should:

Support all commonly used atomic operations.
Aim to not go overboard with too many methods.
Aid in correct use of epochs by leveraging lifetimes and pins/guards.
Allow operations on raw pointers and/or operations that don't require a pin/guard.

The current API succeeds on all points except the last one. Regarding that one, I'd like to avoid having methods like load_raw, store_raw, swap_raw, cas_raw, cas_weak_raw. That adds a lot of new methods. And what about cases where you want to do swap with a raw pointer, but get back a Shared? Or CAS where the previous value is a raw pointer, but the new is Shared?
Instead of creating special methods aimed at raw pointers, I think converting raw pointers to Shared is the way to go, especially in presence of pointer tagging. For example:
let a: *mut T = _;
let b: Shared<T> = _;

// Bad:
let c: *mut T = atomic.cas_raw((a, 1), (b.as_raw(), b.tag()), SeqCst).0;

// Better:
let a = Shared::from_raw(a);
let c: *mut T = atomic.cas(a.with_tag(1), b, SeqCst, pin).as_raw();

The only *_raw method in the current proposal is load_raw. And it is odd. So why does it exist? It exists because loading atomics without pinning in destructors (Drop impls) is very useful. But this begs the question: do destructors need raw pointers or do they need to avoid pinning? Of course, the latter is really true.
All concurrent data structures go through three phases:

Construction (e.g. new and from_iter).
Sharing among multiple threads.
Destruction (drop).

During construction and destruction we have a &mut to the whole data structure and would like to avoid pinning the current thread. At the moment, Crossbeam pins the current thread during construction of MsQueue and during destruction of Deque.
Perhaps it would make sense to create a "fake" pin/guard during construction and destruction in order to pass it to methods on Atomic that require it. Perhaps we shouldn't think of pin/guard as "proof that the current thread is pinned", but rather "proof that nothing will be destroyed in this scope".
When constructing/destructing we know that nothing will be concurrently destroyed under our feet. Perhaps it'd make sense to instead provide an unsafe function that gives a pin/guard, but rather than pinning the current thread just asserts we have ownership of the whole data structure? Then the destructor for Stack might look like this:
impl<T> Drop for Stack<T> {
    fn drop(&mut self) {
        let pin = unsafe { &epoch::assert_ownership() };

        let mut curr = self.head.load(Relaxed, pin);
        while !curr.is_null() {
            let next = curr.deref().next.load(Relaxed, pin);
            drop(Box::from_raw(curr.as_raw()));
            curr = next;
        }
    }
}

If we introduce that function, there'll be no need for store_owned nor compare-and-set. It'll also allow us to freely use Shared<T> instead of (*mut T, usize). Thoughts?

The average tower height is 1 + 1/2 + 1/4 + 1/8 + ... = 2. So, if all nodes have a tower of height 32, while the average is 2, that's a lot of wasted space.

Not that it matters at all for this discussion, but just for factual correctness, I'm pretty sure that this is incorrect. That infinite sum is the probability of the height falling in the range [0, +infty]. If you ignore height 0 (impossible), it's correctly probability 1. The actual average height is (lg n), so for 65536 elements it would be height 16, which is sometimes used as the constant limit. With that limit the wasted space would be n(16-lg(n)), which.. I agree is still pretty bad.
fn load<'p>(&self, ord: Ordering, _: &'p Pin) -> Shared<'p, T, Z>;
fn store<'p>(&self, ord: Ordering, new: Shared<'p, T, Z>);

It's an interesting idea. On the other hand, the cost of untagging is negligible compared to all the sync operations that structures do. I was originally opposed to merging Atomic and TaggedAtomic since the resulting API would be really clunky with tags everywhere, but the new design separates them out neatly. Adding trait hackery to separate types would only make it more confusing.

Users can have pointers to misalinged data.

This is one concern I'm not sure the importance of. I've never seen data being purposefully misaligned for any reason, but if there are exotic cases where it matters, that might be worth considering. If there aren't, I'd agree with @stjepang that the current form, i.e. having just one form of pointer is preferable.
impl<'p, T> Shared<'p, T> {
    pub unsafe fn deref(&self) -> &'p T;
    pub unsafe fn as_ref(&self) -> Option<&'p T>;
}

What does deref do? If it's just as_ref().unwrap(), then it's really unnecessary to have it.
I split the more important point into a post below.

Not that it matters at all for this discussion, but just for factual correctness, I'm pretty sure that this is incorrect.

Oh, I think we're just talking about different things.
I'm saying: sum(tower_heights) / len(tower_heights) = 2.
You're saying: max(tower_heights) = log(n).
Anyways, total wasted (unused) memory is (MAX_LEVELS - 2) * len(tower_heights) * size_of::<usize>().

This is one concern I'm not sure the importance of. I've never seen data being purposefully misaligned for any reason, but if there are exotic cases where it matters, that might be worth considering.

I'm not aware of any either. If there are legitimate situations for misaligned data, it sounds like one is doing something so low-level that they'd probably want to avoid Atomics entirely anyway.

What does deref do? If it's just as_ref().unwrap(), then it's really unnecessary to have it.

It is similar to as_ref().unwrap(), but it is also zero-cost: it doesn't panic when the pointer is null.

After digesting this a little, I've come to the conclusion that it would be best to make Owned nullable and remove Shared::null, reinstating the status of Shared as proving the existence of a pinned epoch.
Here is more or less why:

I think it's important for this rework to provide an API that's equally as expressive as the previous, i.e. allowing for all the things that were possible before, as long as it's not an unreasonable maintenance burden.
It was previously possible to store a value in an Atomic without pinning the epoch. Chase-Lev uses this, albeit wrongly. It could be just Atomic::new. Nevertheless, the possibility existed.
To solve this, we could simply reintroduce store_owned. But notice that since Owned is not nullable, we have to use store with a Shared to pass a null. So to set Some, we use one method, to set None we use another. This does not seem right.
On the other hand, if we made Owned nullable, Shared::null wouldn't have to exist - Owned::null.into_shared would achieve the same thing, but also require a pin, restoring the status of Shared as a pin guarantee.
Thanks to this, we could get rid of all the extraneous Pin arguments where Shared is also an argument.

Since Shared is now nullable, it can be created without a pin by calling Shared::null(). In other words, the existence of a Shared used to be proof that the current thread is pinned. But now it isn't anymore, so methods like cas_and_ref must take a pin/guard.

The above would no longer be true, but I claim that that's for the better. The API would be somewhat cleaner (less arguments in a bunch of functions), but more importantly it would reflect the concepts behind epoch-based reclamation more clearly.
Another indicator of Shared::null being problematic is that the pin argument on Owned::into_shared is unnecessary! Shared does not assert a pin existing, so it should be possible to create it from anything, not just null. The function should then be Owned::into_shared(self) -> Shared<T>.. but oh wait, lifetimes won't allow it. To me this seems like a strong indicator that we're going in the wrong direction with allowing unpinned Shareds.

Perhaps it would make sense to create a "fake" pin/guard during construction and destruction in order to pass it to methods on Atomic that require it. Perhaps we shouldn't think of pin/guard as "proof that the current thread is pinned", but rather "proof that nothing will be destroyed in this scope".

This is an interesting idea, but I find it really confusing. I believe that we should stick to the concepts begin epoch-based reclamation and let our API reflect them as best as possible. A pin asserts a pinned epoch, and a shared pointer is contingent on that pin. The usage of lifetimes in this case really nicely enforces this idea and we should not diverge from it. Allowing a non-pinned Shared essentialy invalidates the whole purpose of the type.
The "fake" pin would also require an internal flag in the Pin to notify it not to unpin on destruction, but that's probably simple enough to do.
The changes to the API would be:
impl<T> Atomic<T> {
+  pub fn store_owned(&self, new: Owned<T>, ord: Ordering);

-   pub fn swap<'p>(&self, new: Shared<T>, ord: Ordering, _: &'p Pin) -> Shared<'p, T>;
+   pub fn swap<'p>(&self, new: Shared<'p, T>, ord: Ordering) -> Shared<'p, T>;

-  pub fn cas<'p>(
-        &self,
-        current: Shared<T>,
-        new: Shared<T>,
-        ord: Ordering,
-        _: &'p Pin,
-    ) -> Result<(), Shared<'p, T>>;

+  pub fn cas<'p>(
+        &self,
+        current: Shared<T>,
+        new: Shared<'p, T>,
+        ord: Ordering,
+    ) -> Result<(), Shared<'p, T>>;

... same for other CASs
}

// having to get rid of these is slightly annoying
// an alternative would be to have Option<Owned> arguments, but as I understand
// we do not want Options everywhere
- impl<T> Deref for Owned<T> { ... }
- impl<T> DerefMut for Owned<T> { ... }

impl<T> Owned<T> {
+  pub fn null() -> Self;
+  pub fn is_null(&self) -> bool;
+  pub fn as_ref(&self) -> Option<&T>;
}

@Vtec234 If Shared is not nullable, how do you do this: atomic.cas(Shared::null().with_tag(1), a, SeqCst)?

I very much like @stjepang 's idea on faking guard.  It will greatly simplify the interface.  FWIW, it is already used in https://github.com/crossbeam-rs/crossbeam/blob/master/src/epoch/participants.rs#L57

On the naming convention, I still strongly feel it would be better to follow the libstd convention.  Personally I am not particularly dissatisfied with long names, e.g. compare_and_swap_weak_owned.  Also, the conflation of the library APIs is currently in an acceptable level.  We can document the differences among the methods and a guideline to choose the right one for the user.  Anyway the file will be less than 300 LoC (i.e. https://github.com/jeehoonkang/crossbeam/blob/cfb4d88387659117281ac8ab4da806ce4920f542/src/epoch/atomic.rs).
For the naming convention, I don't think we can come closer to each other in asynchronous conversation in GitHub issues.  I will try my best to keep online in IRC (irc.mozilla.org #crossbeam).. :)

Okay, this time I'd like to take a step back and perhaps try with new terminology. Perhaps a fresh story might sound more coherent.
Forget everything you know about Crossbeam...
So, epoch-based GC has this Atomic<T> type. It is an atomic pointer to a heap-allocated T. Loading an Atomic<T> returns a pointer Ptr<'scope, T> that can be dereferenced (if it isn't null). Example:
let a = Atomic::new(7); // Atomic pointer to a heap-allocated number 7.
let p = a.load(SeqCst);

But this doesn't work just yet! Epoch-based GC is all about scopes. When working with heap-allocated objects, we don't want other threads to concurrently destroy them under our feet. Let's say we'd like to print the loaded number:
let p = a.load(SeqCst);
println!("{}", *p.as_ref().unwrap());

Between the load and println we really don't want some other thread to free the allocated number 7. There's a way to say to the GC: "please don't destroy any garbage created during the life of this scope before it ends":
epoch::pin(|scope| {
    let p = a.load(Seqcst, scope);
    println!("{}", p.as_ref().unwrap());
})

Now, if another thread decides to free the heap-allocated number at a time between the load and println, freeing will be simply deferred. The garbage collector keeps track of such scopes and will destroy it later at a safe time.
All Ptrs have a lifetime in their type: Ptr<'scope, T>, which indicates the scope in which they're usable.
We can summarize the lives of Atomics and Ptrs like this:

Atomics are long-lived and usually live on the heap.
Ptrs are short-lived and live on the stack within a scope.

The sample code still doesn't compile because as_ref() is unsafe. Dereferencing is safe only if you use memory orderings to properly synchronize writes/reads to/from heap-allocted objects. This will compile:
epoch::pin(|scope| {
    let p = a.load(Seqcst, scope);
    println!("{}", unsafe { p.as_ref() }.unwrap());
});

If you're wondering why we're calling unwrap() here, that is because the pointer might be null. Conversion to a reference succeeds only if it is not null.
How do we deallocate heap-allocated objects? This is how:
epoch::pin(|scope| {
    let p = a.load(SeqCst, scope);
    if !p.is_null() {
        // Try "stealing" the object from the atomic pointer.
        if a.compare_and_swap(p, Ptr::null(), SeqCst, scope).is_ok() {
            // Success! Add the object to the garbage collector.
            unsafe { epoch::defer_free(p, scope) };
        }
    }
})

Function defer_free is unsafe because we must guarantee that the object is now unreachable from atomic pointers. Since we "stole" the object by compare-and-swapping the pointer to something else (null), we know that other threads can't obtain it again.
Some threads might still be holding a pointer to it in their scopes, but as soon as their scopes end, no threads will be able to have a pointer to it again.
Scopes created by epoch::pin must be short-lived, otherwise the GC might get stalled for a long time and keep growing too much garbage.
Sometimes, however, we'd like to have longer-lived scopes in which we know our thread is the only one accessing atomics. This is true e.g. when destructing a big data structure, or when constructing it from a long iterator. In such cases we don't need to be overprotective because there is no fear of other threads concurrently destroying objects. Then we can do:
unsafe {
    epoch::unprotected(|scope| {
        // Destruct all nodes in a Treiber stack.
        let mut curr = head.load(Relaxed, scope);
        while !curr.is_null() {
            let next = curr.deref().next.load(Relaxed, scope);
            drop(Box::from_raw(curr));
            curr = next;
        }
    })
}

Function unprotected is unsafe because we must promise that no other thread is accessing our atomics.
Just like with the safe pin function, unprotected use of atomics is enclosed within a scope so that pointers created by it don't leak out or get mixed with pointers from other scopes.
The only remaining question is: how do we store new objects into Atomics? For that, there is type Owned<T>, which is almost identical to Box<T>. In fact, Owned<T> can even be constructed directly from Box<T>.
We can convert an Owned<T> into Ptr<'scope, T>:
let mut node = Owned::new(Node {
    value: value,
    next: Atomic::null(),
});

epoch::pin(|scope| {
    let n = node.into_ptr(scope);
    // ...
}

Or, we can use compare-and-swap to install new objects into atomic pointers. Here's how one might push a new owned node onto a stack:
epoch::pin(|scope| {
    let mut head = self.head.load(Acquire, scope);
    loop {
        owned.next.store(head, Relaxed);
        match self.head.compare_and_swap_weak_owned(head, owned, AcqRel, scope) {
            Ok(_) => break,
            Err((h, o)) => {
                head = h;
                owned = o;
            }
        }
    }
})

That's all!
By the way, can we make as_ref() a safe method if we do these two things?

Make compare_and_swap_{,weak_}owned use Release if Relaxed was given, and use AcqRel if Acquire was given.
Issue a Release fence in Owned::into_shared.

@jeehoonkang, what do you think? Do the memory orderings check out?

I think this is solid, don't mind my crazy type system suggestions.
:+1: for pin::exclusive and stdlib like (long) names.

All right, I've updated the RFC with the recent changes. Thanks everyone for the feedback and your inputs! We're getting very close to finalization, but a lot has changed recently so I'd like to let everything sink in a bit, at least for a few days.
It seems that at this point only bikeshedding remains: Should epoch::unprotected be renamed to epoch::exclusive? Or perhaps something else?
I'll try to summarize what everyone thinks on that:

@Vtec234 says that epoch::exclusive is more explanatory, since it asserts that we have exclusive access to the data structure.
@jeehoonkang feels that the name exclusive doesn't capture the fact that actually there can be a concurrent pin or exclusive. He prefers unprotected, without_pin, or unsafe_pin. In other words, exclusive and pin don't sound like the two opposites. But he will not object to exclusive and doesn't have a strong opinion.
I slightly prefer epoch::exclusive because it evokes what we're asserting (that we have exclusive access) rather than what is happening in the scope (unprotected use of atomics).

Unless a better suggestion comes along or someone argues against it, it'll be renamed to epoch::exclusive.
@aturon You don't have to read the lengthy discussion, but if you could skim through the RFC, that'd be super appreciated! After all, you designed the original interface and have lots of experience with API design in Rust, so your opinion would be very valuable here. :)

One could suggest epoch::local() as well, but I'm ok with epoch::exclusive()

Here's an argument against naming it pin::exclusive...
Yes, the function is useful in situations where we know that our thread is the only one accessing  atomics.
But, it is also useful in situations where we simply know that no thread (including our own) is modifying atomics.
An example can be found in the implementation of deque in Coco. In method push we need to load the pointer to the buffer. Pinning is unnecessary because we know that our thread is the only one that is able to modify the pointer, and we can promise that we won't modify it at this time.
Therefore, it's not about exclusive access to atomics. It's about exclusive-writer or multiple-reader access. Kind of like a RwLock or RefCell. The function is safe to use only if either of this is true:

There is exactly one writer (our thread) and no readers.
There are no writers.

@aturon

One very minor question: you seem to drop Atomic::new, instead having the user allocated an Owned to initialize. Any particular reason for that?

Good catch! This was not intentional. I'll add Atomic::new to the interface.

@schets Can we merge, or would you like more time to review?

It looks good, I have one idea - if we're providing the unprotected epoch function, would it also make sense to provide an unsafe re-pin function which leaves and re-enters the epoch without leaving the scope? The use case I imagine is where you

Need a protected pin
Need a long-running pin
Have a segment where you know the current thread doesn't have unsafe references.

 It would be great in situations where it's hard to maintain a state machine outside of the pinned context.

@schets That's a good point - in some cases repinning might be very handy.
Do you perhaps have a suggestion on how exactly would such a function be used?
Here are some quick ideas:
epoch::pin(|scope| {
    // Idea 1:
    unsafe { epoch::repin(); }

    // Idea 2:
    unsafe { epoch::repin(scope); }

    // Idea 3:
    unsafe { scope.repin(); }
});

It's a bit unfortunate that we can call this function even inside an epoch::unprotected scope. We could solve the problem by having different types like PinScope and UnprotectedScope that implement trait Scope, but that is probably too much complexity for too little benefit.
I personally am not sure what's the right interface here. More concrete cases where this is needed would be illuminating.
If we don't figure out the interface now, we can mention this in the "Unresolved questions" section and decide after gaining some experience with the library, or when a concrete need for this feature comes up.

I suggest postponing that for now.

Added the problem of re-pinning to unresolved questions, since it's not blocking the current proposal and we can figure out it later.
This RFC is being merged. Great work, everyone!
This week I'll publish a new RFC on epoch-based garbage collection, containing a comprehensive overview of everything related to it. This is a wide design space requiring lots of tradeoffs, and solutions to some problems are not obvious at all.


Cannot join stake-pool on shelley_testnet

Context

Information
-

Version
55ffb6b4abcbc7f7ab15c60d2bc4b1c61208cfff anviking/fix-pools-for-testnet

Platform
Linux

Installation
From source

Steps to Reproduce
https://hydra.iohk.io/job/Cardano/iohk-nix/cardano-deployment/latest/download/1/index.html

cardano-node run \
        --config /root/node/$type/*-config.json \
        --database-path /root/node/$type/db \
        --socket-path /root/node/$type/node.socket \
        --topology /root/node/$type/*-topology.json \
        --host-addr <IP_ADDRESS> \
        --port 3001}

cardano-wallet-shelley serve --port 8090 \
                --node-socket /root/node/$type/node.socket \
                --testnet /root/node/$type/*-genesis.json \
                --database /root/wallet/$type/db

Join stake-pool on shelley testnet with wallet having funds: (fuel gentle purse blood bomb advice hundred vehicle maze special pull category smart control shallow)

PUT http://localhost:8090/v2/stake-pools/1fd1450ad92fc024235e2bf908a9b178a6e13f2e653ff0dec5414c46/wallets/98ae2ce26143a126a19a2d47e60b539a596b4d2b
Body: {"passphrase":"Secure Passphrase"}

Expected behavior
I can join.
Actual behavior
Code = 500,
Message = {"code":"created_invalid_transaction","message":"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: ApplyTxError [LedgerFailure (UtxowFailure (UtxoFailure (ValueNotConservedUTxO (Coin 994831859) (Coin 995231859))))]"}

Resolution

QA

Seems like we are missing a required deposit needed when registering a stake key. This wasn't caught by our integration tests because the value for key deposit is set to 0 in our cluster genesis!


Expression Mapper Implementation

Ability to use functions and expressions within mapping logic. for example, concat(), etc

At the moment, there is no function contribution model for mappings. The only supported functions should be implemented within the expression mapper

@lixingwang : Lets start on porting mapper capabilities.

Fixed at https://github.com/TIBCOSoftware/flogo-lib/pull/127


⚠ Thumbnails API Endpoint has degraded performance

In d7260ae, Thumbnails API Endpoint (https://thumbnails.roblox.com/v1/assets?assetIds=82738847&format=Png&isCircular=false&size=30x30) experienced degraded performance:

HTTP code: 200
Response time: 5373 ms

Resolved: Thumbnails API Endpoint performance has improved in 48017c7 after 11 minutes.